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

Run on plugins_loaded to help fix PR #752 #884

Closed
wants to merge 1 commit into from

Conversation

carstingaxion
Copy link
Collaborator

@carstingaxion carstingaxion commented Sep 18, 2024

This PR should help with the one big step, which is part of #752, loading the whole plugin on plugins_loaded.

This became an urgent need to make l10n work properly, like described in detail by @swissspidy in #752 (comment).

Description of the Change

Closes #

How to test the Change

Changelog Entry

Added - New feature
Changed - Existing functionality
Deprecated - Soon-to-be removed feature
Removed - Feature
Fixed - Bug fix
Security - Vulnerability

Credits

Props @username, @username2, ...

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

Copy link

Preview changes with Playground

You can preview the least recent changes for PR#884 by following one of the links below:

⚠️ Note: The preview sites are created using WordPress Playground. You can add content, edit settings, and test the themes as you would on a real site, but please note that changes are not saved between sessions.

@carstingaxion
Copy link
Collaborator Author

carstingaxion commented Sep 18, 2024

@mauteri This is the bare-bones testing PR, to help with the new load order, we talked about.

I tested locally, that when I also add the changes from 3c2230a (which belongs to https://github.com/carstingaxion/gatherpress/tree/fix/651) to that branch, the failing tests go down from 21 to 6.

But those 6 are the same as in #752

@mauteri
Copy link
Contributor

mauteri commented Sep 19, 2024

@carstingaxion I thought about this more and I think the issue is one of the hooks in one of the singletons being at wrong priority and maybe that is what we need to explore and not wrapping everything in plugins_loaded. All code in those singletons are executed in a hook, even class-setup.php. That class instantiates and runs setup_hooks. It also instantiates the other singletons and they do the same thing; they run their setup_hooks. So the issue is in one of those hooks and that would be the fix rather than executing ALL hooks in plugins_loaded.

I can look at your previous PR and we can discuss next week and I can help you identify the issue you are running into and which hook is responsible. Thx!

@carstingaxion
Copy link
Collaborator Author

carstingaxion commented Sep 22, 2024

@carstingaxion I thought about this more and I think the issue is one of the hooks in one of the singletons being at wrong priority and maybe that is what we need to explore and not wrapping everything in plugins_loaded. All code in those singletons are executed in a hook, even class-setup.php. That class instantiates and runs setup_hooks. It also instantiates the other singletons and they do the same thing; they run their setup_hooks.

Thank you for the explanation. In general this is not new to me and this structure is one of the reasons I like GatherPress so much - its easy to understand & read. I like it.

So the issue is in one of those hooks and that would be the fix rather than executing ALL hooks in plugins_loaded.

I can look at your previous PR and we can discuss next week and I can help you identify the issue you are running into and which hook is responsible. Thx!

Yes @mauteri, I guess this will be the path and I think we might start at #752 (comment):

4. and was able to track it down until `Assets->localize()`, which calls
   `'timezoneChoices'  => Utility::timezone_choices(),` and seems to be the culprit. Disabling this line brings all translations back into shape; having this enabled messes things up.

@carstingaxion
Copy link
Collaborator Author

As things are not complicated enough, I believe that I may have tried to prevent or workaround this 8 year old problem (without knowing about) https://core.trac.wordpress.org/ticket/38643 reported by ... checks notes ... @swissspidy

Sometimes it's wild.

@carstingaxion
Copy link
Collaborator Author

I was not able to reproduce the former bug, so closing this as not needed anymore.

Thats annoying, when you've spent plenty of time .... for nothing.

@carstingaxion carstingaxion deleted the try/run-on-plugins_loaded-to-fix-PR752 branch October 5, 2024 15:41
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