Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
We added unnecessary setters to the LanugageInterface. The whole point of the Language object is that it is not configurable. The ConfigurableLanguage object exists for that purpose.
Proposed resolution
- This issue removes all setters from the LanguageInterface
- Removes implementations of such setters from Language and ConfigurableLanguage
Remaining tasks
Review patch.
User interface changes
None.
API changes
- Removal of all setters from Language and ConfigurableLanguage
Comment | File | Size | Author |
---|---|---|---|
#17 | 16-17-interdiff.txt | 1.45 KB | alexpott |
#17 | 2334763.17.patch | 15.07 KB | alexpott |
#16 | 2334763.16.patch | 15.77 KB | alexpott |
#16 | 1-16-pseudo-interdiff.txt | 3.7 KB | alexpott |
#1 | 2334763.1.patch | 21.48 KB | alexpott |
Comments
Comment #1
alexpottHere's a patch.
Comment #2
Gábor HojtsyUpdated issue summary, it said setValuesFromValues while it is setValuesFromLanguage.
I am wondering that now that so much was done to unify the two interfaces / implementations, why do we need the intermediary Language object when we save a new ConfigurableLanguage?
Also re the locked property, a ConfigurableLanguage may not be in fact configurable on the UI IF it is locked. Drupal core ships with 2 non-configurable languages as config entities (AKA ConfigurableLanguage :D). Naming things is hard right? :)
Comment #3
alexpottRemoving incorrect statements about the locked property from the issue summary.
Comment #4
alexpottI think we don't need it either. Created #2336177: ConfigurableLanguage cannot be created/removed directly, integrate language_save() and language_delete() to address this.
Comment #5
Gábor HojtsyYeah this looks like an amazing intermediary step then :) I am a huge proponent of not doing everything in one issue :)
Comment #6
Gábor HojtsyWait, the method_id stuff. core/modules/language/tests/language_test/language_test.module still uses it to test if the right method was used and that is not affected by this patch. Also surprising it passes with removal of that. How can that be?
Comment #7
alexpottI'm postponing on #2336177: ConfigurableLanguage cannot be created/removed directly, integrate language_save() and language_delete() since that makes the need for
ConfigurableLanguage::setValuesFromLanguage()
obsolete :)Comment #8
YesCT CreditAttribution: YesCT commentedI wish we had #2057905: [policy, no patch] Discuss the standards for phpunit based tests and #2253915: [policy, no patch] Fix coding standards for PHPUnit tests in a better state.
Comment #9
YesCT CreditAttribution: YesCT commentedmaybe a separate issue to handle
+++ b/core/lib/Drupal/Core/Language/LanguageDefault.php
@@ -53,7 +53,7 @@ public function get() {
* The default language.
*/
public function set(LanguageInterface $language) {
- $language->default = TRUE;
+ $language->setDefault(TRUE);
$this->language = $language;
}
default will be a protected in #2226533: Changes to the Language class due to the LanguageInterface (followup) and this issue takes out the setDefault().
Maybe we should not have the default property? since... a recent discussion has said that if a language is default is not stored on the language object. #2336177: ConfigurableLanguage cannot be created/removed directly, integrate language_save() and language_delete()
Comment #10
YesCT CreditAttribution: YesCT commentedsimilar #2337835: set language->default in ConfigLanguageOverrideWebTest
Comment #11
YesCT CreditAttribution: YesCT commentedsimilar #2337843: LanguageEditForm take out set name and direction, use create() parameters
Comment #12
YesCT CreditAttribution: YesCT commentedslightly related to an irc conversation we had about language tests: #2337943: make a language test base helper that makes a random langcode (not in scope of this issue though)
Comment #13
martin107 CreditAttribution: martin107 commentedI am not expressing an opinion one way or the other, but just to say I appreciate an informed discussion,
I have a use case for keeping some get/setters in \Drupal\language\Entity\ConfigurableLanguage
namely setDirection() and setName()
please see https://www.drupal.org/node/2337843#comment-9146131 which discusses how to transform \Drupal\language\Form\LanguageEditForm::submitForm()
Comment #14
Gábor Hojtsy@martin107: #2336177: ConfigurableLanguage cannot be created/removed directly, integrate language_save() and language_delete() makes much more sweeping changes, no need for set methods. See there.
Comment #15
Gábor HojtsyThat just landed, so this needs a reroll.
Comment #16
alexpottYep - we no longer need ConfigurableLanguage::setValuesFromLanguage() :)
Still need to remove changes that removed the language method property since this is used and with the current code I think this is the one setter we need.
Comment #17
alexpottSo there were no "real" usages of the method_id getter and setter methods. So I've not added them back. I've added the public properties out - we can completely remove the method_id in another issue. I noticed that property names were not actually equivalent anyway show that the current code is a bit of mess anyways. This patch makes the property names the same to make the refactor easier and the code in LanguageNegotiationMethodBase::persist() at least consist for Language and ConfigurableLanguage.
Comment #18
Gábor HojtsySo language negotiation would only happen if language module is turned on, right? Otherwise all language types are resolved to the default language. In which case can a Lanaguage object (as opposed to ConfigurableLanguage) be result of language negotiation and therefore need the method_id?
Comment #19
Gábor HojtsyOh wait, I see it was on both classes BEFORE the patch. I guess that was for API compatibility because the getter/setter was on the interface. Since you want to remove this anyway, there is little point debating it on this issue, and you just restored a non-modified situation regarding those properties. So good then. Let's get this in too :)
Comment #21
catchCommitted/pushed to 8.0.x, thanks!
Comment #22
Gábor HojtsyYay, thanks!
Comment #23
tstoecklerWhy exactly was stuff like
setWeight()
removed fromConfigurableLanguage
? I agree that it doesn't make sense onLanguage
, but onConfigurableLanguage
? In the unit tests, the calls tosetWeight()
are being replaced with e.g.new ConfigurableLanguage(['weight' => 0])
which is far worse DX than$language->setWeight()
. I didn't check how the UI sets the language weights.I would like to open an issue to revert that change but am wondering if there are some specific arguments that I am missing here.
Comment #24
alexpottThe UI uses
set->('weight', $value)
. I'm not opposed to adding setters to ConfigurableLanguageInterface - but if it is only being used in tests what is the point - just more code to maintain.Comment #25
tstoecklerWell, in my book
setWeight() > set('weight')
, but sure I'll open a follow-up, it's not a big deal either way. Just wanted to know if there was anything I missed. Thanks for the clarification!Comment #27
YesCT CreditAttribution: YesCT commented#2341341: Change public 'name' property access on languages to getName() and add back setName() is adding back a setName()