The 2026 Drupal Business Survey is open. Your response will help shape the future of the Drupal business ecosystem.Problem/Motivation
The constructor of the Permissions class currently requires the second parameter to be an instance of a TranslationManager class. However, in general, you should aim to program against an interface and not a concrete class (i.e not violating the dependency inversion principle). Because of this, possible other implementations of a TranslationManager (added by other modules) will cause this to fail.
Proposed resolution
In stead of type hinting the concrete class used, we should use an interface as type-hint (following the dependency inversion principle). In this case that would be the TranslationInterface.
Remaining tasks
- Create a patch
- Review patch
User interface changes
None
API changes
See proposed resolution. This should not break existing implementations, as they all will comply to the TranslationInterface. However, this will allow other implementations of the TranslationInterface to be used as well.
Data model changes
See proposed resolution. This should not break existing implementations.
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | entity_browser-use_interface-3100254-10.patch | 1.47 KB | bhupesh_upadhyay |
| #2 | entity_browser-use_interface-3100254-2-D8.patch | 1.37 KB | dimiter |
Comments
Comment #2
dimiter commentedActually this patch should be attributed to bonrita! Many thanks!
Comment #3
sandeep date commentedComment #4
berdirMaybe just use StringTranslationTrait and then use setStringTransalation and $this->t().
Comment #5
dimiter commented@Berdir That might be a good alternative. However, I deliberately kept the changes as small as I possibly could, to prevent unwanted side-effects from being introduced. Perhaps your suggestion could be a new issue on its own?
Comment #6
eelkeblokI would also suggest to take this chance and make it more standards-compliant. Although theoretically there might be some derived class somewhere that depends on the protected member $translationManager, chances of that are slim. Other than that, the impact is minimal. Is it possible to deprecate a member variable? In that case you could even deprecate $translationManager but also assign it the injected TranslationInterface, for maximum compatibility.
Comment #7
bonrita commentedComment #8
bonrita commentedComment #9
anybodyI think #4 is the better solution.
TranslationInterface has no
getStorage()method, so IMHO the patch is wrong.Comment #10
bhupesh_upadhyay commentedHi, I have created a patch, please review this.
Comment #11
bhupesh_upadhyay commentedComment #12
benstallings commentedThe StringTranslationTrait provides getStringTranslation() which falls back to \Drupal::translation() if no translation service has been explicitly set. So the current code already works — $this->t() resolves fine without injection.
For that reason, I'd call this a feature request instead of a bug report.
Comment #13
berdirThis used to be a bug, but that was fixed by #3156523: Permissions::__construct is hinting with a class, so closing as duplicate.