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

hookable LanguageSupportPageNames::getPagePath() #94

Open
kixe opened this issue May 14, 2017 · 2 comments
Open

hookable LanguageSupportPageNames::getPagePath() #94

kixe opened this issue May 14, 2017 · 2 comments

Comments

@kixe
Copy link

kixe commented May 14, 2017

Short description of the enhancement

hookable LanguageSupportPageNames::getPagePath()

Current vs. suggested behavior

Currently LanguageSupportPageNames::getPagePath() is not hookable whereas Page::path() is.

Why would the enhancement be useful to users?

I am currently working on a complete rewrite of the Multisite module (single database).
https://github.com/kixe/Multisite/tree/kixe
https://processwire.com/talk/topic/1025-multisite/?page=10#comment-144509
To modify the page pathes I decided to hook in Page::path() instead of modifying the output of Page::render(). The module works already quite stable, but needs to have LanguageSupportPageNames::getPagePath() hookable for full language support.

@ryancramerdesign
Copy link
Member

I try to avoid adding hooks unless it's something where there really is no existing path to modify the result of a function. That's because there's a certain amount of overhead associated with every hook call, and functions like path() get a lot of calls. It's not much overhead at all, but added up over hundreds or thousands of calls, it can be a concern. Are you able to hook after Page::path(), perhaps with priority level 200, which would ensure it get called after the Page::path() hook in LanguageSupportPageNames.module? i.e.

$this->addHookAfter('Page::path', $this, 'hookPagePath', ['priority' => 200]); 

If that still doesn't achieve what you need, let me know what specifically you need from the getPagePath() method you want to hook, and I can see if there's some way to get to it. If not, I can make it conditionally hookable, which should help to prevent any overhead except when hook is active.

@kixe
Copy link
Author

kixe commented Feb 1, 2018

Its not only the Page::path() which is modified by LanguageSupportPageNames::getPagePath().

IMPORTANT
LanguageSupportPageNames::verifyPath() compares the $_GET['it'] (modified by the Multisite module) with the path returned by LanguageSupportPageNames::getPagePath(). If the path doesn't match the function returns the 'correct' path and a redirect is forced by LanguageSupportPageNames::ready()
To prevent unexpected redirects I have to modify both $_GET['it'] and the return value of getPagePath().

Furthermore there are other functions calling LanguageSupportPageNames::getPagePath() directly and not via Page::path().

  • Page::localUrl()
  • Page::localPath()
  • Page::localHttpUrl()

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