WIP: BUGFIX: Create a 410-Gone redirect for deleted nodes #32
Conversation
| { | ||
| // By default the redirect handling for removed nodes is activated. | ||
| // If it is deactivated in your settings you will be able to handle the redirects on your own. | ||
| // For example redirect to dedicated landing pages for deleted campaign NodeTypes | ||
| if ($this->enableRemovedNodeRedirect) { | ||
| $hosts = $this->getHostnames($node->getContext()); | ||
| $this->flushRoutingCacheForNode($node); |
There was a problem hiding this comment.
Is it unproblematic that the routing cache is not informed about the removed node anymore?
There was a problem hiding this comment.
Not sure, it would have to be informed at the collect stage anyways since the node is not there anymore during redirect creating.
Also i assume that other components are informing the routing cache that a node was deleted already. Maybe @kitsunet can tell.
| | user-testaccount | | ||
| And I remove the node | ||
| And I publish the workspace "user-testaccount" | ||
| And I should have a redirect with sourceUri "en/about.html" and statusCode "410" |
There was a problem hiding this comment.
| And I should have a redirect with sourceUri "en/about.html" and statusCode "410" | |
| Then I should have a redirect with sourceUri "en/about.html" and statusCode "410" |
|
You could try this (test is untested 😄 ) |
|
@gerhard-boden not creating redirects for hidden nodes is problematic because of this feature here https://github.com/neos/redirecthandler-neosadapter/blob/master/Tests/Behavior/Features/Redirect.feature#L97-L104 Imho the check should be inverted here to verify that no redirect was created. |
|
yes, rest of the tests might still be outdated ofc, since we only decided yesterday how to handle hidden nodes 😄 I don't see an issue in dropping / rewriting that test case. I updated my code sample above yesterday after the discussion. |
|
I wouldn't worry about breakiness either, we never claimed anywhere that it was an explicit feature that we create redirects for hidden nodes and it's a pretty useless feature too in retrospective... |
mficzel
force-pushed
the
bugfix/createGoneStatusForDeletedDocuments
branch
from
6db2a25
to
350707c
Compare
|
@gerhard-boden I added/adjusted the tests for hidden documents. We should ask @nezaniel about wether removing the flushing of the router cache is safe since he added it with #21. I consider this obsolete since the new architecture should allow to let the routing cache be handles solely by the \Neos\Neos\Routing\Cache\RouteCacheFlusher. |
| if ($node->isVisible() === false) { | ||
| return; | ||
| } | ||
|
|
||
| if (!$this->hasNodeUriChanged($node, $targetWorkspace)) { |
There was a problem hiding this comment.
This hasn't changed with this PR, but I run into this condition all the time, when I'm trying to test the redirects for deleted nodes.
Seems to me that a deleted node does not necessarily come with a detectable URI change.
mficzel
force-pushed
the
bugfix/createGoneStatusForDeletedDocuments
branch
from
350707c
to
f48432a
Compare
mficzel
changed the title
|
Stopped working for now since i discovered that the adapter behaves differently in behat. The previous change only worked in behat but not in neos. I pushed a new changeset that only contained the additional tests as a base to pick this up again later. |
|
I close this since the problem somehow is deeper than i thought |
If a node cannot be read in the createPendingRedirects stage it is considered to have been deleted and the removeNodeRedirect method is triggered.
I addition the restrictions are applied in the collect stage to work on removed nodes but
also to work on the origin before the publishing as specified in the docs.
fixes: #31