rapila / cms-base

The rapila cms’ internals. Please file all issues here if they’re not directly related to a plugin or the sample site.
http://www.rapi.la
3 stars 1 forks source link

PageObjectFillHelper fillContainerWithModule() executed also when NOT FOUND #239

Closed juergmessmer closed 6 years ago

juergmessmer commented 6 years ago

PageObjectFillHelper fillContainerWithModule() tries to fill content even if the page is not found. The page is not redirected to error_404.

juergmessmer commented 6 years ago

This concerns only release-1.7: It's because the DefaultPageType is not modified here.

juergmessmer commented 6 years ago

I tried to cherry-pick the missing commit "Extract page fill logic from DefaultPageTypeModule" from into release-1.7: dd1a7c052cc02f9eb22c58b771e1c03bb837ff47 But it's not enough...

sabberworm commented 6 years ago

Did you try switching between master and release-1.7 on the same site or do you simply have two separate sites, one on master and the other on release-1.7? If the latter, can you test with the former set up?

Because what you’re describing makes sense: the error page is never redirected to, it is simply displayed when a 404 is encountered. So if the error page is of the default page type, it is rendered like any other page, using fillContainerWithModule. AFAIK this is not a bug.

juergmessmer commented 6 years ago

I switched on the same site (wettingen portal) and it works with master. release-1.7 includes new PageObjectFillHelper but not all the changes that have been implemented in master

On 24 May 2018, at 08:07, Raphael Schweikert notifications@github.com wrote:

Did you try switching between master and release-1.7 on the same site or do you simply have two separate sites, one on master and the other on release-1.7? If the latter, can you test with the former set up?

Because what you’re describing makes sense: the error page is never redirected to, it is simply displayed when a 404 is encountered. So if the error page is of the default page type, it is rendered like any other page, using fillContainerWithModule. AFAIK this is not a bug.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub, or mute the thread.

sabberworm commented 6 years ago

Can you describe what exactly the bug is?

juergmessmer commented 6 years ago

the navigation item in PageObjectFillHelper line 96 is null $aPath = $this->oNavigationItem->getLink();

On 24 May 2018, at 08:07, Raphael Schweikert notifications@github.com wrote:

Did you try switching between master and release-1.7 on the same site or do you simply have two separate sites, one on master and the other on release-1.7? If the latter, can you test with the former set up?

Because what you’re describing makes sense: the error page is never redirected to, it is simply displayed when a 404 is encountered. So if the error page is of the default page type, it is rendered like any other page, using fillContainerWithModule. AFAIK this is not a bug.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub, or mute the thread.

sabberworm commented 6 years ago

I have cherry-picked 10586e379da1e38fe425dd378ec63a5b9a9b9537 from master to release-1.7. Does this fix the issue?

juergmessmer commented 6 years ago

I guess, but strangely enough I still encounter the error with a site running on base release-1.7. On any other site this works without problem.

sabberworm commented 6 years ago

Could it be that this one site also has a frontend module on the error page that uses the current navigation item?

juergmessmer commented 6 years ago

What do you mean by 'has a frontend module "on the error page"?'

This site uses the JournalPlugin and EventPlugin, they both use navigation items. I don't see what it could be. Neither in EventsFilter nor in JournalFilter any items are created at the specific request. Logging some test strings in Default and JournalPageType suggests that the DefaultPageType is instanciated twice: once with the root page and then with this not existing navigation item.

[Thu May 31 16:12:37.887097 2018] [php7:notice] [pid 910] [client 127.0.0.1:49714] ([0] => 'DefaultPageType', [1] => 'Home') [Thu May 31 16:12:37.901023 2018] [php7:notice] [pid 910] [client 127.0.0.1:49714] ([0] => 'DefaultPageType', [1] => '-') [Thu May 31 16:12:38.104301 2018] [php7:notice] [pid 910] [client 127.0.0.1:49714] ([0] => 'fillContainerWithModule', [1] => true)

That's where the exception is thrown. Funny enough the browser displays the 'error-404' page's name, but the NotFoundFrontendModule is not executed, though it's the only active module of the page. And I removed all journal or event related content from the parent "root" in order to check if it makes a difference.

I also checked the NotFoundFilter, but I don't see any relationship to that one either.

juergmessmer commented 6 years ago

How come the page is redirected to error-404 but still tries to handle a navigation item that does not exist?

sabberworm commented 6 years ago

You do have an error-404 page?

You have some frontend modules on it?

Does one of them use the current navigation item?

juergmessmer commented 6 years ago

No, on page error-404 there is only the NotFoundFrontendModule configured that simply displays the path that has not been found. This page inherits root's container contents, but even if I remove all the contents in root, this strange exception remains...

sabberworm commented 6 years ago

Do the sites that work have the same set-up (NotFoundFrontendModule used on error-404 page)?

Does the site that does not work also work on master (where you initially said the problem was fixed)?

Does the problem still occur on $aPath = $this->oNavigationItem->getLink();? If so, in what file is this called?

juergmessmer commented 6 years ago

Now it seems resolved, no clue why. I checked out all the master branches and cleaned up the versions (rebased some forked issues, merged bugfixes 1.7 into master), checked master (everything ok), checkout all release-1.7 versions, and now: everything works!

sabberworm commented 6 years ago

So we can close this?

juergmessmer commented 6 years ago

I guess so.