Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fatal error when a DS redirects to 404 #1539

Closed
vlad-ghita opened this issue Nov 9, 2012 · 22 comments
Closed

Fatal error when a DS redirects to 404 #1539

vlad-ghita opened this issue Nov 9, 2012 · 22 comments

Comments

@vlad-ghita
Copy link
Contributor

[2.3.1] I have a Datasource that will redirect to 404 if no entries found and Symphony throws a fatal error:

http://postimage.org/image/s2b99jtyz/

Ideas?

@brendo
Copy link
Member

brendo commented Nov 9, 2012

I can't reproduce on a 2.3.1 install.

  • Do you have a 404 page defined?
  • Do you any custom redirects on the 404 URL?
  • Have you modified any of the error templates?
  • Have custom error_reporting settings?
  • What version of PHP do you have installed?

@designermonkey
Copy link
Member

Neither can I.

Also,

  • Are the permissions on the class.errorhandler.php correct for your Apache PHP user to access it?

@vlad-ghita
Copy link
Contributor Author

Do you have a 404 page defined?

Yes

Do you any custom redirects on the 404 URL?

Just a plain 404 Page

Have you modified any of the error templates?

No

Have custom error_reporting settings?

No

What version of PHP do you have installed?

5.3.13

Are the permissions on the class.errorhandler.php correct for your Apache PHP user to access it?

644

Check out these links. First one doesn't exist.

http://xandercms.mcas.ro/en/no-page-here/

This is a page with a title parameter and a DS attached to it that filters based on $title param and redirects to 404 if no results found.

http://xandercms.mcas.ro/en/products/home/

@brendo
Copy link
Member

brendo commented Nov 10, 2012

Does the error exist without the Language Redirect extension?

@vlad-ghita
Copy link
Contributor Author

I don't have Language Redirect. I'm using Frontend Localisation. It doesn't redirect like the former did.

@brendo
Copy link
Member

brendo commented Nov 14, 2012

Regardless, does the error persist with the extension disabled? Just trying to see if there is a conflict.

@brendo
Copy link
Member

brendo commented Nov 15, 2012

You don't happen to have that DS attached to the 404 page as well do you? Looking at the database query log it looks like the 404 page is found correctly, but then the DS is executed again.

@brendo
Copy link
Member

brendo commented Jan 9, 2013

bump

@vlad-ghita
Copy link
Contributor Author

Will check tomorrow (around 9:00 AM GMT). I don't have file access at the moment.

@jurajkapsz
Copy link
Contributor

I stumbled on the same error and it happened when I had the same DS which throwed 404 due to empty results, attached also to the 404 page itself. Removing the DS from 404 page stops this error. Followed here.

@brendo
Copy link
Member

brendo commented Jan 10, 2013

And to completely recreate the scenario, the reason that DS failed was because URL parameters were not passed to the DS when the original 404 occurred.

  • DS attached to page /abc/:param/:param2, has throw 404 set
  • 404 page, /errors/404/ has the same DS attached, but the /:param/:param2 is not passed to this page, so the page 404's again
  • Recursion.

Possible solution suggested by @creativedutchmen was to pass the whole path to the 404 page anyway so the /errors/404/ path would be /errors/404/abc/. This would require the 404 page to have parameters, possibly /:page/:param/:param2 set.

It'd work, but it could get pretty messy very quickly. Consider if you had a URL /article/read/:id/:handle. Passing this to a 404 page that only has /:page/:param/:param2 would fail, as /:handle would be orphaned. I think this route encourages bad design, where users could potentially stuff the 404 page with /:param/:param2/:param3 etc to handle all situations.

What if we just caught the recursion?

@nilshoerrmann
Copy link
Contributor

I vote for just catching the recursion.
But what about adding the original path to the 404 page's XML? This way, developers could at least do something with the initial URL data in their 404 template.

@creativedutchmen
Copy link
Member

@brendo the reason to suggest the parameters to be added to the 404 page is to be able to tailor 404 pages for different pages.

For instance, a 404 because a blog post was not found could show similar posts, while a 404 on an author profile could show related authors.

I initially misunderstood the problem, I thought this was what he wanted to achieve. So I agree it is not a solution to the recursion. However, having the URL information in the XML could be very useful.

edit: ah, yes, ofcourse Nils beat me to it...

@jurajkapsz
Copy link
Contributor

In case of my situation of this error, I wanted to take care only about one, content returning, DS. But the scenario-based 404 (e.g. blog, products etc.) sounds very interesting.

I try to add: Ok, besides catching the recursion, what about to delegate the page's original params to the 404 page by Symphony's functions (e.g. Frontend::Page()->_param...)? That is whatever parameters were found on the original page - maybe to give them some prefix, so developers would need to create a different DS for that.

The URL of 404 page would stay the same as the original page URL - which is nice that way - and wouldn't have to be further processed.

Edit: The former URL in XML is fine.

@jurajkapsz
Copy link
Contributor

Well, its kind of weird to recreate those params, if they did not returned anything before ... but the developer could do some proper action with that I guess ... or maybe its silly ... nah, sorry, thinking aloud :/

Anyway, I am sure you do the right thing :)

@brendo
Copy link
Member

brendo commented Jan 14, 2013

So while fixing this issue tonight I noticed something. If you are redirected from a page that throws a 404, the $current-path parameter will be filled on the 404 page with the path that caused the 404 error, meaning that you can already do what you like with the URL on your 404 page to provide custom messages :)

@brendo
Copy link
Member

brendo commented Jan 14, 2013

Ok, a debrief:

  • There was actually a regression since Symphony 2.3 where Datasource's where not handling FrontendPageNotFoundException's correctly, this has now been resolved and will be active upon resaving your datasources.
  • There was a bug that has existed for as long as I can see where 404's thrown when a parameter is required were not handled using the FrontendPageNotFoundExceptionHandler, this has been resolved.
  • When the 404 page contains a datasource that throws a 404, the recursive 404 is caught and a standard Symphony error page is shown with the message This error occurred whilst attempting to resolve the 404 page for the original request.
  • There was talk of adding the original requested parameter into the XML. At the moment I can't see how that might happen as when the recursion is caught, we are throwing a Symphony exception and not parsing an XSLT page. Saying that seems to highlight a limit, a 404 page cannot have a DS that throws a 404 on it as it will incur recursion.
  • On the original 404 page, the *-path parameters will contain the originally requested path.

Any ideas?

@nilshoerrmann
Copy link
Contributor

That sounds to me as if you resolved everything that needed to be resolved.

@brendo
Copy link
Member

brendo commented Jan 14, 2013

Almost, but those who are using Datasources on their 404 page that throw 404's will still be in a spot. At the moment I can't think of a solution for this scenario. We 'could' look to see if a recursive exception has been set, and conditionally throw the FrontendPageNotFoundException (essentially ignoring the throw statement) and let the rest of the page render as normal. Feels ugly though.

@michael-e
Copy link
Member

My 2 cents: If you need "highly customized" 404 pages, you can still use normal pages and build an extension to set a 404 header for these pages under certain conditions.

@brendo
Copy link
Member

brendo commented Jan 14, 2013

Good point, maybe I'm overthinking it :) And FYI, the original issue in the first post has been resolved, the processParameters function's throw is now caught.

@nilshoerrmann
Copy link
Contributor

What about only enabling the Data Source redirection on normal pages, not on error pages. I doesn't make sense to redirect to a 404 page when you are already on it – so I would catch this case in the Data Source class before the actual recursion happens.* We could then notify the developer about the attempt to redirected to the error page in the Symphony logs. Something like:

You attempted to redirect to your 404 error page recursively. Please check your 404 page's Data Source settings.


* Something like if($page-type !== '404') redirect …

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants