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

Allow global context to be assumed instead of 404ing #10326

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kennydude
Copy link

I've been working on getting some 404 handling working via a plugin in order to provide a better experience, and to be able to drop in redirects on an article by article level as we transition towards OJS within OICC from.

However, when OJS decides on the context, it defaults to 404ing if the context couldn't be locating, meaning we can't hook in here and do anything so I've looked at tweaking it to default to global context, allowing for 404s to be handled by any handler registered.

I didn't find much luck any other way following on the forum https://forum.pkp.sfu.ca/t/better-404-handling/89573

@asmecher
Copy link
Member

Thanks, @kennydude! The handle404 call is supposed to end execution, so I don't recommend bypassing it. Maybe it would be better to introduce a hook in the handle404 function so that plugins can inhibit the normal operation and e.g. present the alternative 404 page instead?

@kennydude
Copy link
Author

I did try adding a hook there, however without a context set I couldn’t get my plugin to load as technically nothing is enabled with no context. It seems to be mostly where we want 404s on OJS with multiple journals effectively for a publisher.

@asmecher
Copy link
Member

Ah, there's a circular dependency here: in order to load the plugins we have to find out what context we're in, so a plugin that's enabled in a context (journal) won't be loaded in order to respond to a 404.

I think it might simply not be possible to have a plugin handle 404s outside a journal unless we change the plugin load model, which I'm hesitant to do. Maybe it would be better to add support for config file options for custom pages, e.g. a section like this in config.inc.php:

[errors]
# Use 404-sitewide.html for 404 requests outside any particular journal
error_page[404][index] =  "/path/to/404-sitewide.html"
# Use 404-for-publicknowledge-journal.html for a single journal with the path "publicknowledge"
error_page[404][publicknowledge] =  "/path/to/404-for-publicknowledge-journal.html"

# (Incompatible with the above) Set a global 404 page
error_page[404][*] =  "/path/to/404-all.html"

But I'll ping @ewhanson on this, as he's been tinkering with an error page for OJS, and might have a counterproposal.

@kennydude
Copy link
Author

That could work. My only extra spanner into this, is I’ve been also looking at redirecting an old URL structure.

I.e hook onto 404, search if the article can be found in the database (there’s 1000s otherwise htacess might have been), redirect to the OJS article URL

@asmecher
Copy link
Member

@kennydude, hmm, do many of these prexisting URLs fall outside the scope of a single journal's path? Or is that an outlier case that can be handled another way?

@kennydude
Copy link
Author

Pretty much all of them (100s, one per article as the URL structure is entirely different from our Wordpress multi site), otherwise I would have probably just used a htaccess tweak in order to do the job 😅

@asmecher
Copy link
Member

@kennydude, hmm, this is a pretty niche need -- you might be best off with a straight-up modification to the handle404 function in the near term. In the longer term, we'll be adopting more of Laravel's approach to error handling -- there's a bit of initial discussion of this over here, and if we can steer it in a way that'll help you in the future, let us know.

The other option I can think of is to use an auto_prepend_file to handle redirects (probably based on some kind of regular expression set) before OJS runs. It would be executed on every request, but you could make it run pretty quick.

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

Successfully merging this pull request may close these issues.

2 participants