enonic / xp

Enonic XP
https://enonic.com
GNU General Public License v3.0
202 stars 34 forks source link

Cannot trigger 404 when filter applied #8530

Closed rymsha closed 3 years ago

rymsha commented 3 years ago

https://github.com/enonic/xp/issues/8417#issuecomment-706027199

if the request is made for a 404 resource, next(req) will just return the closest matching content. So a request for /page-exists/missing-page should give us the 404 (and trigger any handle404 in error.js) but instead returns the page for /page-exists/ (meaning it is impossible to get to the 404 page). Maybe I should open a new issue for that particular behaviour?

https://github.com/enonic/xp/issues/8417#issuecomment-726733265

Also if you try a random URL on this website, you will find it impossible to trigger the build in error404 handler. You will see the closest parent content instead. Which is totally not what I expected =) This URL for example doesn't exists, but it will not show the 404-page (unless I deactivate/delete my filter) it will show the news-page. http://localhost:8080/admin/site/preview/default/draft/my-corporation/news/skjfhskdjhf%20kasjhdflkjas%20dhf

rymsha commented 3 years ago

@sigdestad any comment on this?

espen42 commented 3 years ago

May or may not be the same issue:

When trying to set up a catch-all-existing-content mapping like for example…

<mapping controller="/lib/external-frontend/previewProxy.js" order="98">
    <match>type:'.+:.+'</match>
</mapping>

…it also makes that controller handle paths to non-existing content - effectively overriding 404 behavior for that site.

When calling portalLib.getContent in that controller, for a path to non-existing content, it returns / falls back to the root site item.

sigdestad commented 3 years ago

That is defineltly a bug

rymsha commented 3 years ago

It looks like the same issue Introduced by this commit https://github.com/enonic/xp/commit/968e47c626e0d654 as desired functionality: for missing content MappingHandler resolves first available parent content and memoizes for rendering (if controller or filter allows it)

We'll fix it by rolling back this "feature" - so controllers/filters will still "see" first available content, but rendering won't happen (same as it would be without mapping controller/filter)

rymsha commented 3 years ago

After a second thought with @sigdestad we decided to remove recursive resolution of content to "first available". URL pattern that matches unresolvable content will still work (filter/controller with pattern /a/.* will still BE CALLED for /a/b/missing) but content won't be resolved anymore (Will be null). This will make content resolution with and without mapping filters work equally.

Applications that relied on old (now considered invalid) behavior should either use getSite or implement own logic to get some "fallback" content.