2.x Fix menu location compatibility with WPML #2733
Conversation
gchtr
added
2.0
Ready for Review
|
Hello @mcaskill, I have mixed feelings about adding another helper class. I'm actually trying to get rid of them. I don't use WPML, isn't there a way to use WordPress core filters to handle this? Could you give it a try? |
|
Yeah, I was reluctant to adding another helper class but couldn't think of a quicker solution at the time. I'll try to refactor this in the next month or so. There should indeed be a way to hook into the |
|
It would be great if we could find an easier way to fix this. Moreover, I think that a fix for this doesn’t have to make it into the first 2.0 release. It looks like we can still solve this with a minor release. To me, it looks like if we find the right filter, this can be fixed without having to introduce a breaking change to Timber. That’s why I’m changing the label to |
gchtr
added
2.x Future
and removed
2.0
Ready for Review
mcaskill
force-pushed
the
2.x-wpml-nav-menu-locations
branch
from
5f3bd19 to
54b8516
Compare
|
I've updated the initial pull request description to reflect the latest proposal. The one outstanding portion of this proposal is the menu location retrieval methods added to |
mcaskill
force-pushed
the
2.x-wpml-nav-menu-locations
branch
from
54b8516 to
7214059
Compare
mcaskill
force-pushed
the
2.x-wpml-nav-menu-locations
branch
from
7214059 to
043911b
Compare
|
I've rebased the latest changes to the main branch and resolved issues.
Most of the failed checks from GitHub Actions appear to be related to existing issues? |
mcaskill
force-pushed
the
2.x-wpml-nav-menu-locations
branch
from
93f9dde to
ce2bb42
Compare
|
Hi @mcaskill , tests are running again! It seems there are only a few failing tests that needs a fix. Mostly about the moved |
Removed from `MenuFactory` in timber/timber#c59b90e
And allow for shared hooks for customization and third-party integrations. Helpers: - `get_menu_location()` to find a location given a menu. - `get_menu_locations()` to retrieve all locations and menus. Filters: - "timber/menu_helper/menu_locations" to filter the locations and menus. Replaced usage of `get_nav_menu_locations()` with `MenuHelper` equivalents in `Menu` and `MenuFactory`.
Added a callback on the 'timber/menu_helper/menu_locations' hook to translate the menus mapped to locations. Without this filter, any attempts to retrieve a menu's location will return `null` preventing one from customizing Timber's `Menu` class. WPML only translates `get_nav_menu_locations()` in specific use cases such as when its filtering `wp_nav_menu_args` and when we are in the Admin.
Replaced callback on the proposed 'timber/menu_helper/menu_locations' hook with the 'theme_mod_nav_menu_locations' hook to translate the menus mapped to locations. Without this filter, any attempts to retrieve a menu's location will return `null` preventing one from customizing Timber's `Menu` class. WPML only translates `get_nav_menu_locations()` in specific use cases such as when its filtering `wp_nav_menu_args` and when we are in the Admin.
Changed: - Moved methods from `MenuHelper` to `Timber` and replaced occurrences of predecessor. - Removed filter "timber/menu_helper/menu_locations".
Amends 8d03809 Proposed changes to WPML integration requires filtering locations from `theme_mod_nav_menu_locations` which is called by `get_nav_menu_locations()` which was used by the mock WPML function.
Changed: - Missing method on `MenuFactory` was moved to `Timber` class.
mcaskill
force-pushed
the
2.x-wpml-nav-menu-locations
branch
from
9afe8c6 to
5c97a1f
Compare
|
Hi @Levdbas, I've rebased and fixed the tests. Thanks. |
Issue
On projects using WPML and that customize the
Menuclass via thetimber/menu/classmapfilter, the custom classes won't be used because WPML only translatesget_nav_menu_locations()(theme_mod_nav_menu_locations) in specific use cases such as during the filtering ofwp_nav_menu_argsand when we are in the Admin.The (translated) menu's term ID won't match the (default language) menu ID assigned to the same location.
This affects
MenuFactoryandMenu.Solution
Since the issue affects multiple classes, and based on suggestions:
theme_mod_nav_menu_locationshook inWpmlIntegrationto translate the menus mapped to locations.get_menu_location()andget_menu_locations()toTimberfor convenient usage byMenuFactoryandMenu.Impact
Adding an extra hook could affect performance but I should be minor.
Translating the menu IDs of menu locations (
theme_mod_nav_menu_locations) could have a more noticeable impact on performance but should also be minor.Usage Changes
Externally, developers can take advantage of the added hook to fix WPML's edge case.
Timber and developers can rely on the new methods to easily retrieve filtered nav menu locations.
Testing
Unit tests have not added for
Timber, nor has the unit tests forMenuFactory,Menu, andWpmlIntegrationbeen updated.With the custom
wpml_object_id_filter()function declared in the tests bootstrap, adding/updating tests will require more time than I have at the moment.This pull request's functionality has been tested on a client project that has many menu locations and customizes the menus extensively.