Updated: Comment #0
Problem/Motivation
#1862202: Objectify the language system moved most of the language negotiation code to classes. The language entity CRUD API, which is rarely used, was held back to limit the size of the patch.
Proposed resolution
- Move language entity CRUD API to a dedicated
LanguageStorageManager
service. - Inject it where appropriate.
Remaining tasks
- Decide whether keeping BC procedural wrappers.
- Write a patch
- Fix failing tests
- Reviews
User interface changes
None
API changes
TBD
Comment | File | Size | Author |
---|---|---|---|
#39 | drupal_2166923_39.patch | 17.05 KB | kfritsche |
Comments
Comment #1
plachThe parent issue has been committed.
Comment #2
larowlan/me takes a look
Comment #3
larowlanassuming you mean language_{save,delete}
left behind old wrappers and didn't convert anything, lets see what the bot says.
Comment #5
andypostSuppose language.crud better name for that. There's path.crud already in core services
Looks like not CRUD ;)
Comment #6
larowlanmisused StorageController::create()
Comment #7
kim.pepperWe're using Storage and Repository as well in a few places too. I think CRUD is the odd one out.
Comment #8
larowlanGreen, so just a matter of names from here.
Comment #9
jibranSome minor issues. Do we need unit test here? so that we can retire some simpletests.
we can use inheritdoc here.
White spaces not correct.
Comment #10
kim.pepperJust a matter of names? Surely there is a 100 comment bike-shedding opportunity here. ;-p
Comment #11
larowlanI actually wrote 'Let the bikeshedding begin' and then took it out.
Comment #12
martin107 CreditAttribution: martin107 commented6: language-crud-2166923.2.patch queued for re-testing.
Comment #14
martin107 CreditAttribution: martin107 commentedComment #15
kim.pepperRe-rolled and fixed the issues in #9.
Comment #16
plachThis is looking good, thanks! Just one remark:
We have no use statement for these. Also I think it would make sense to add an
instanceof
check.Also, could we try and see whether removing BC procedural wrappers would increase the size of the patch unreasonably?
Comment #18
kim.pepperConfigurableLanguageManager is in the same namespace, so no need for a use statement. Not sure what you mean about instance of? Do you mean on $this->languageManager? If so, looking at the service.yml, it's not an instance of ConfigurableLanguageManager.
I've fixed up the call to variable_set() and converted some \Drupal::service() calls to injected services. Looks like this patch was quite old.
Comment #19
kim.pepperI looked at removing BC wrappers, and there is a tonne of calls to them, including in the installer. I'd suggest we make that a follow up.
Comment #20
jibranok then RTBC.
Comment #21
plachD'oh! Silly me :)
Service implementations can be swapped so the actual language manager instance stored in
$this->languageManager
may or may not beConfigurableLanguageManager
, that is the one the Language module and that replaces the core language manager. In that case we would be calling therebuildServices()
method on an object that does not define that method. So, yes, I really think we should add the following check to to those ifs:Comment #22
kim.pepperI understand how dependency injection works, but sorry, that still doesn't make sense.
Do we want to call ConfigurableLanguageManager::rebuildServices() *only* if the injected LanguageManager happens to be an instance of ConfigurableLanguageManagerInterface? I thought we'd want to do that all the time?
Comment #23
plachWe want to call it only if it's actually there: we cannot assume every implementation of the language manager will need to rebuild services. The proper fix should probably be making
::rebuildServices()
non-static, moving it onto theConfigurableLanguageManagerInterface
, then checking whether the language manager in an instance of CLMI.Comment #24
kim.pepperThanks for the explanation.
I've applied fixes as per your recommendations in #23.
Comment #25
plachAwesome, thanks!
Comment #26
alexpottThis has been renamed in #2188613: Rename EntityStorageController to EntityStorage
Comment #27
XanoComment #28
kfritscheChanges seems reasonable and fine.
Setting back to RTBC.
Comment #29
alexpottThis is not the correct format.
I also wonder if more of this should be moving into the language entity?
Also this patch is incorrectly rerolled given #2223249: Remove hook_language_insert/update/delete/presave in favour of entity hooks
Comment #30
alexpottAll this has been removed
LanguageStorage::languageStorage??? Super confusing. And is why I wonder if more of this should be moving into the language entity?
Comment #31
kfritscheFixed deprecated messages and the wrong re-roll mentioned by alexpott.
Also renamed the languageStorage to entityStorage. I hope this is more clear than languageStorage.
Comment #32
kfritschePatch is just some minor things which alexpott also told me, which seems incorrect.
* In other entities (e.g node) watchdog messages for CRUD is happening in form controllers, moved that there
* If we are in the language module, we are sure that we have the ConfigLanguageManager, so removed the if statements there too
There is still discussion going on here and I just wanted to summarize the latest status I heard of and that we at the end don't forget about this smaller stuff.
* LanguageStorage doesn't make sense as it is no storage at all. At maximum is a language to language entity mapper
* One idea was to move the methods from current LanguageStore to the ConfigurableLanguageManager - plachs concern is that we then adding a circular dependency
* Other idea is that the LanguageStore is extending the ConfigEntityStorage and change the methods accordingly and the Language entity is using this storage then
Comment #33
plachActually we can't be sure we are dealing with a
ConfigurableLanguageManager
instance: another module could swap-in a different implementation for the language manager service (see #23).Comment #34
kfritscheFixed #33.
Also after talking with plach we decided to extend the ConfigEntityStorage for the LanguageStore. So we don't have a special service for that anymore. Everything related to that is now in the language entity storage, which seems more reasonable.
Hope this is now better? Otherwise we also would be fine to do it in another way, if this still doesn't seem good.
Comment #35
alexpottThis should move into the postSave method so all LanguageEntity saves do this.
This needs to move to postDelete
Comment #36
kfritscheLooks much better now, thanks.
I moved the noted stuff to the language entity postSave and postDelete methods.
This is still in the LanguageStorage, as we don't have this field in the language entity. So its still something special when you save the Language instead of the language entity, but I believe this is okay. As the main idea is, that cache get also cleared when language entities are imported or similar and these can't have the default field.
Comment #37
alexpottThis is not quite that right. Firstly we don't need to do this for an update. Plus we only need to do this if the language_manager has become multilingual by adding more than one language.
Comment #39
kfritscheSome tests still fail, this has to be further investigated, but I have to leave for now.
But fixed the most tests, because of a missing use...
Regarding #37:
Reworked this a little bit, like it was before. It is now only get fired if a new language gets added and if the site has now 2 languages.
Comment #40
kfritscheComment #42
martin107 CreditAttribution: martin107 commentedComment #43
kfritscheIs this still needed?
Most of the stuff is done in the ConfigurableLanguage, see #2336177: ConfigurableLanguage cannot be created/removed directly, integrate language_save() and language_delete().
Otherwise this can be closed.
Comment #44
sidharrell CreditAttribution: sidharrell commentedIs this still active? Does it still need a reroll?
Comment #45
alexpottThis has been fixed by #2336177: ConfigurableLanguage cannot be created/removed directly, integrate language_save() and language_delete()