Problem/Motivation
Currently Address overrides cores country_manager service in such a way that other modules cannot user this service as it is documented in the API.
See this issue on the Ubercart issue queue.
Ubercart does not alter the first (and by default only) argument to the country_manger service which is of type ModuleHandlerInterface, but Ubercart does add a second argument to the country_manager service. It appears that the Address module also alters the country_manager service. However, the core Drupal country_manager service requires that the first argument to the CountryManager constructor is of type ModuleHandlerInterface.
Proposed resolution
Despite the official documentation stating that the methods used by this module are correct, but:
Note that while swapping services is very easy, you should use caution when making use of this feature. If multiple modules swap the same service, then neither module can predict if their swapped service will win eventually. If a service provides extension capabilities with events, hooks, or by other means, that is definitely more compatible with other extensions.
It seems that perhaps the better solution would be to employ service decorators: Symfony | Lullabot | Phase2
From Lullabot:
The problem arises when there is another module that does the same thing. In that situation either you are replacing that module’s spiced entity manager or that module is replacing your improved alternative. There is no way to get both improvements at the same time.
This is the type of situations where the decorator pattern comes handy. You cannot have this new module inheriting from your manager, because you don’t know if all site builders want both modules enabled at the same time. Besides, there could be an unknown number of modules –even ones that are not written yet– that may create the conflict again and again
Remaining tasks
TBC
API changes
TBC
Data model changes
TBC
Comments
Comment #2
cbovard CreditAttribution: cbovard commentedThank you for adding this!
Comment #3
bojanz CreditAttribution: bojanz at Centarro commentedThank you for raising this issue.
A selling point of this module is a more up-to-date country list with translations for over 200 languages.
We replace the country manager so that the same country list is available to all of Drupal, not just the Address form element and field type.
I've looked at the Ubercart class in question: http://cgit.drupalcode.org/ubercart/tree/uc_country/src/CountryManager.p...
They are obviously adding functionality that's not a part of the original CountryManager contract, which would make a lot more sense on an Ubercart specific service. However, it seems that they are also trying to accomplish the same thing we are: replacing Drupal's country list with their own source (though in a non-performant way, every Uc Country is its own config entity, hello 200 entity loads).
Now that Address has its own country field type and form element, I'm comfortable with telling people to just use those if they want access to a better country list. The few (admin-facing) core examples can stay on the old list.
So let's just stop overriding CountryManager and make the life of Ubercart users easier.
Comment #5
bojanz CreditAttribution: bojanz at Centarro commentedComment #6
cbovard CreditAttribution: cbovard commentedThank you for your time on this!!!
Comment #7
dkre CreditAttribution: dkre commentedReally appeciate this fix. Thank you!