CreateSnapshotAdminExtension bug fix #1709
Conversation
c-Rolland
force-pushed
the
bug-fix-CreateSnapshotAdminExtension
branch
from
e82e80a
to
d43d698
Compare
c-Rolland
force-pushed
the
bug-fix-CreateSnapshotAdminExtension
branch
from
d43d698
to
6fa1707
Compare
| @@ -77,7 +77,7 @@ public function update(SiteInterface $site, ?OutputInterface $output = null, boo | |||
| if ( | |||
| !$this->decoratorStrategy->isRouteNameDecorable($name) | |||
| || !$this->decoratorStrategy->isRouteUriDecorable($route->getPath()) | |||
| || (null !== $routeHostRegex | |||
| || (\is_string($routeHostRegex) | |||
There was a problem hiding this comment.
null !== $routeHostRegex
&& '' !== $routeHostRegex
&& ...
Will fix the psalm error
| if ($object instanceof PageInterface) { | ||
| return; | ||
| } | ||
|
|
||
| $this->createByPage($object); |
There was a problem hiding this comment.
I'm not familiar with this lib, maybe you can explain to me the change behind ?
SOmething like:
"This extension is used to create a snapshot of a Page. When you persist/update a Page or a PageBlock you want to update the snapshot of the Page. But when you remove something, the snapshot should only be updated if we remove a PageBlock. When we removed a Page, the snapshot should just be deleted with the page" ?
There was a problem hiding this comment.
It works on my side. It prevent the snapshot to re-create the page on page deletion.
|
At the time of my old PR, the fix was targeting Now the extension @VincentLanglet I didn't checked it yet but it seems wrong to me: in case of a page removal, no snapshot would be created? |
I don't know ; I never used this lib. |
|
Could you please rebase your PR and fix merge conflicts? |
|
if it is fixing a bug, I would say it should have a test covering this bug. it will help us to understand better what is the issue and what you are fixing. |
|
hi , what is blocking on this MR ? it looks like it is solving my issue on my side. |
| if ($object instanceof PageInterface) { | ||
| return; | ||
| } | ||
|
|
||
| $this->createByPage($object); |
There was a problem hiding this comment.
It works on my side. It prevent the snapshot to re-create the page on page deletion.
Hi @GeraudBourdin @thomasBourdin. Some context was need for me to understand why the PR #1293 was kinda revert here. The explication is now known. The goal of the link PR was to fix the snapshot when deleting PageBlocks. This PR is trying to solve things when deleting Pages. What's missing now would be:
If any of you want to resubmit a PR with a green Ci, I would be happy to merge it. |
|
Solved in #1721 |
Subject
I am targeting this branch, to fix a bug of CreateSnapshotAdminExtension::postRemove() method.
Closes #1708.