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

Improve 404 behavior for MultiViews=true|false #164

Open
philip opened this issue Apr 2, 2022 · 5 comments
Open

Improve 404 behavior for MultiViews=true|false #164

philip opened this issue Apr 2, 2022 · 5 comments

Comments

@philip
Copy link

philip commented Apr 2, 2022

A feature request to improve 404 behavior as it relates to MultiViews support.

Three options exist for this feature request, either:

  • (A) Provide a functioning alternative URL in the 404 result to the desired content
  • (B) Similar to (A), but redirect to the functioning URL instead (302)
  • (C) Or, allow paths for both multiViews=true|false to function simultaneously

All options would provide a smoother transition from one method to another. Today I attempted a move to mutiViews=true but we have many old links (in old bug reports, for example) that turn into blank 404 pages so we feel stuck keeping it false.

The contents of these 404 pages (as of WebSVN v2.7.0) for a standard file view are:

  • With multiViews=false: the multiview URLs offer a 404 stating why it's not found, similar to:

MultiViews must be enabled in include/config.php in order to use browse.php. See
the install docs for details, or use this path instead.

Important: the "this path" does not provide a desirable path; instead it's a static value to the root page.

  • With multiViews=true: the non-multiview URLs offer a blank 404, similar to:

WebSVN 404 No repository given.

Ideally each 404 page would link to a functioning URL for the desired content (allowing options (A) and (B)) but (C) would be an okay but potentially more complicated option.

All hints for implementing the simpler (A) option are welcome! There are a lot of renderTemplate404() references and contexts; and thus far I got it working for basic file viewing to replace 'WebSVN 404 NOREP' but wonder if there's a simpler way. Would moving operation logic out of multiviews.php make sense for this?

@philip
Copy link
Author

philip commented Apr 2, 2022

Here's a rough idea for comment; this is not a PR :) This hack works for our use case in that it redirects old style URLs for file views and revision comparisons. It's not well-tested nor does it consider other reasons a person might see a 404 here. Seems the URL detection and URL construction logic should be centralized but that'd require more time.

philip@7643919

@ams-tschoening
Copy link
Contributor

Could you please provide some concrete examples for URLs you have not working with MultiViews? Additionally make clear how those would need to look like to work with MultiViews, corresponding to your suggestions of automatic redirects. Maybe you could add some kind of table with existing URL, result with MultiView enabled/disabled and how a redirect should look like?

URL Result MV enabled Result MV disabled Rewrite
http://example.org/... 404 no repo 404 MV disabled http://example.org/...

Redirecting automatically if possible at all seems to make sense to me. Though one should keep in mind that web servers might handle redirecting "wrong" URLs themself already, e.g. using .htaccess. From the point of view of WebSVN this might mean that it doesn't need to handle anything special at all and users might have redirects outside of WebSVN in place anyway already because of refactored SVN repos hierarchies, renamed repos and stuff.

@philip
Copy link
Author

philip commented Apr 4, 2022

My original request was written before looking at the code. The rough patch for comment (not a PR!) shows an idea of how it'd work and it functions for both file view and file comparisons. I did not edit for other operations. The bug isn't about MV, it's about how non-MV URLs are handled with MV enabled (I did not consider the reverse situation). URLs like so:

With MV: websvn/browse/path/to/file.xml (works)
With MV: websvn/filedetails.php?repname=path&path=path/to/file.xml (404)
Without MV: websvn/browse/path/to/file.xml (404)
Without MV: websvn/filedetails.php?repname=path&path=path/to/file.xml (works)

The desire is for both to styles to work at the same time. With MV enabled, my rough patch adds the functioning MV URL to the 404 page when non-MV URLs are encountered. Redirection here works with header("Location:...) but I left it as a 404 as, in theory, people will update their bookmarks. I plan to make it automatically redirect after a month or so passes.

@ams-tschoening
Copy link
Contributor

I did not edit for other operations.

And that's the whole downside of this approach, different URL types need to be handled in multiple files. I would try to look at something more generic, rewriting URLs at one place if possible, that's why I brought mod_rewrite etc. into discussion. Having something in multiviews.php might make sense as well and might have the benefit of not being specific to one concrete web server. Using PHP instead of mod_rewrite etc. would additionally allow to print error messages like in your case.

Some if* checking if MultiViews is enabled and then providing multiple RewriteRules matching non-MV URLs seems like a good start to me. Though, I didn't find yet how to check if Options MultiViews is enabled per request in .htaccess.

@philip
Copy link
Author

philip commented Apr 4, 2022

Thanks, the limiting factor (for me) is figuring out the URL structure for both multiview and non-multiview styles, and doing so without trial-and-error. But after looking closer today, getURL() and getURLParts() offer big hints as does the $op switch in multiviews.php although that $op switch has fewer specific options than getURLParts(). The 2nd limiting factor (for me) is figuring out where all these variables come from and why they may or may not exist, such as $rep. There are many.

Once figured out, a goal is to analyze the URL and make both styles known to all; and it appears setup.php would be the home (potentially via WebSvnConfig as it contains getURL() today). From there, performing the action would be easy.

So it appears both URL formats could function simultaneously if the logic were centralized and I'll look closer as time permits. The action (e.g., notification of change, or redirect, ...) is the easier part. I'd prefer not adding another requirement and simply do this all in PHP.

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

No branches or pull requests

2 participants