Problem/Motivation
Proposed resolution
Remove setName() from LanguageInterface. Reimplement filterLanguages() with creating a new default language object for the list.
Remaining tasks
Do it. Review. Commit.
User interface changes
None.
API changes
LanguageInterface will not have setName(). It is not supposed to have that logically.
Comment | File | Size | Author |
---|---|---|---|
#39 | 2407125-diff-37-39.txt | 878 bytes | vijaycs85 |
#39 | 2407125-remove-set-name-language-39.patch | 6.19 KB | vijaycs85 |
Comments
Comment #1
Gábor HojtsyComment #2
Gábor HojtsyComment #3
zealfire CreditAttribution: zealfire commented@Gabor, i was trying to solve this issue,but i have some doubts regarding implementation of filter languages().Since we have to remove setName() so do we need to override default language name as it was done by setName() ? I guess removing "$default->setName($this->t("Site's default language (@lang_name)", array('@lang_name' => $default->getName())));" this line would be fine.
Please tell me.Thanks.
Comment #4
Gábor HojtsyThe idea is you would create a
new Language()
with data appropriate for this display variant of the default language only for the runtime.Comment #5
sidharthapI am working on it.
Comment #6
DevElCuy CreditAttribution: DevElCuy commentedComment #7
DevElCuy CreditAttribution: DevElCuy commentedRemoved SprintWeekend2015Queue by mistake.
Comment #8
sidharthapComment #9
Cogax CreditAttribution: Cogax commentedComment #10
Cogax CreditAttribution: Cogax commentedI've made something, but i don't now if it's the solution. If not, please help me to get on the right way. Tanks!
Comment #11
Cogax CreditAttribution: Cogax commentedComment #13
Cogax CreditAttribution: Cogax commentedComment #14
Cogax CreditAttribution: Cogax commentedI updated the Testcases, so there was a test case for the setName method.
Comment #15
mglamanApplies, tests pass. Looks good.
Comment #16
alexpottThis is important - this allows sites to translate
Site's default language
. We shouldn't be removing this. I think Gabor was thinking we should be creating new language object here.Comment #17
Gábor HojtsyIndeed, looks good except:
Right, we want the label to be different here in the runtime. We should use
new Language()
for a language we only use in the runtime.Comment #18
Cogax CreditAttribution: Cogax commentedThanks a lot, i hope i got it now. If not, please help me, i'ts all new to me :)
Comment #19
Gábor HojtsyLooks good.
Comment #20
alexpottThis clarifies the difference between Language and ConfigurableLanguage and it also makes Language a true value object because it is immutable. Nice.
This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 574b0eb and pushed to 8.0.x. Thanks!
Comment #22
alexpottOops missed something - we should be moving LanguageInterface::setName to ConfigurableLanguageInterface since we are not removing ConfigLanguage::setName() - and we shouldn't be removing that.
Comment #24
Gábor HojtsyYeah sorry I don't know how we missed that... Adding some test coverage also to ConfigurableLanguageUnitTest.
Comment #25
Gábor HojtsyDuh patch did not include the changes in #24. Now does.
Comment #28
Gábor HojtsyFails seems to be unrelated in Drupal.php where it gets an entity manager.
Comment #31
Gábor HojtsyHm, related.
Comment #32
Gábor Hojtsy@Cogax: wanna continue with this one?
Comment #33
Cogax CreditAttribution: Cogax commentedSure, I'll try..
Comment #34
Gábor Hojtsy@Cogax: how is it going?
Comment #35
Cogax CreditAttribution: Cogax commentedI could need some help. I searched a lot and dived deeper and deeper into Drupal (that's the only good thing on that) but with no result.
The only thing that i think we should made is that we should implement a setUp() Method for the ConfigurableLanguageUnitTest. I've tried to setup there a Language Entity Manager (wich is needed in the label() Method of Entity.php) like it's been done in some other tests. But the Error still exists.
I had also problems to debug. I run the test alwas in my console with
but i don't know how to make test outputs like print_r / print / echo / var_dump etc. I've written this outputs always in a textfile wich isn't a good soloution..
Comment #36
Cogax CreditAttribution: Cogax commentedComment #37
vijaycs85As mentioned by @Cogax, we need the container to test setName/getName methods. Added new kernel test case which covers this.
Comment #38
Gábor HojtsyYay, thanks, just some cleanup:
These style comments are not used on simpletest based tests, no?
Comment #39
vijaycs85Thanks for the review @Gábor Hojtsy. Here is an update...
Comment #40
Gábor HojtsyLooks good, thanks!
Comment #41
alexpottThis issue is a normal bug fix, and includes only a very small disruptive change that contrib should not have been relying upon, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 4f776f6 and pushed to 8.0.x. Thanks!
Comment #43
Gábor HojtsyThanks all!