As discussed in #1512424: Add a LanguageInterface, and property setters/getters to Language class, after #1497230: Use Dependency Injection to handle object definitions introduced the language class, and made language negotiation return instances of this class, it makes most sense to do the same for language_list() and language_default() (and any other places where we return language, if we know more :). language_load() just reuses language_list() so this should fix that too.
Sounds like this could be as simple as language_list() initializing a new instance of Language with every record and language_default() doing the same based on the variable values. Currently the Language object is "backwards compatible" with the anonymous data objects we used before (as in we still use at places), so we can just pass the values in the constructor and leave the actual data use as-is for now. More would come later as we work out #1512424: Add a LanguageInterface, and property setters/getters to Language class
Comment | File | Size | Author |
---|---|---|---|
#46 | language-class-changelog.txt | 630 bytes | Gábor Hojtsy |
#32 | drupal-language_instance-1627208-32.patch | 6.06 KB | vasi1186 |
#30 | drupal-language_instance-1627208-30.patch | 6.15 KB | vasi1186 |
#27 | drupal-language_instance-1627208-27.patch | 6.39 KB | vasi1186 |
#25 | drupal-language_instance-1627208-25.patch | 6.39 KB | vasi1186 |
Comments
Comment #1
vasi1186 CreditAttribution: vasi1186 commentedComment #2
vasi1186 CreditAttribution: vasi1186 commentedAttached a patch that makes the language_list() and language_default() function return Language objects.
Comment #4
vasi1186 CreditAttribution: vasi1186 commentedlet's try again.
Comment #5
vasi1186 CreditAttribution: vasi1186 commentedComment #6
tstoecklerThere should be a space after the cast. Maybe we can simply fetch arrays directly, that would be cleaner IMO.
Hmm... this looks strange. Instead of casting here can we instead update the code that saves this to directly save an array?
Comment #7
vasi1186 CreditAttribution: vasi1186 commented@tstoeckler:
Agree with one, I'll change it.
For the second, how about changing the constructor of the Language class and make the cast inside it?, something like:
Comment #8
Gábor Hojtsy@vasi1186: We usually try to stay away from APIs that can be invoked with different data structures like that I think. We already have language_default updates in bootstrap.inc and tests for those updates, so changing it to an array should be a few more lines in the update code hopefully :)
Comment #9
Gábor HojtsyOh, and storing an array in language_default would also be great so that nobody mistakes it for a Language class instance. It will not be an object and that should show it :)
Comment #10
sunThe language_default variable bugs me for a long time already. Ideally, it should only contain a langcode string, nothing else. If people want to override or customize the resulting language object, then they should do so properly.
Comment #11
RobLoachThere is no way we could just outright remove the language_default variable, could we? Seems like if you're running Drupal in a different language, you'd have the Language module installed anyway, which has its own handling of the default language.
Comment #12
vasi1186 CreditAttribution: vasi1186 commentedAttached a new patch that should always save the language_default as an array.
Comment #13
vasi1186 CreditAttribution: vasi1186 commentedComment #14
Gábor HojtsyI don't think this would actually change the variable to an array :) This runs in the update and works with the Drupal 7 database, at which point this is still an object. So looks like all the changes here would need to be undone and the variable_set at the end needs the (array) cast and moved out of the condition AFAIS.
Comment #15
vasi1186 CreditAttribution: vasi1186 commentedOne thing to clarify about a new method in the Language class, that is called toArray(). The reason why I created the method is that I would not rely on the defaut cast operation that PHP can do, because in the future this class will have probably lots of changes, and it could happen that some of its attributes will become private. In that case, the cast operation will fail.
Comment #16
vasi1186 CreditAttribution: vasi1186 commentedComment #17
Gábor Hojtsy@sun: the langcode thing has a nice issue at #1272862: Clean up default language handling and can be done regardless of making these things result in instantiated classes.
Comment #18
vasi1186 CreditAttribution: vasi1186 commentedImplemented the changes from #14.
Comment #19
vasi1186 CreditAttribution: vasi1186 commentedThe previous patch was incomplete...
Comment #21
vasi1186 CreditAttribution: vasi1186 commentedPrevious patch failed testing probably because of a small thing in testDependencyInjectedNewDefaultLanguage().
Comment #23
cosmicdreams CreditAttribution: cosmicdreams commentedLooks like you made some progress here I'll try to review and help out so that you guys can have a better starting point tomorrow
Comment #24
tstoecklerMinor, but I would prefer $info here instead of $language.
Same here. $options doesn't really make sense to me.
This deserves a comment.
Can we leave this out for now? If we want kill the 'language_default' variable saving an array anyway...
Comment #25
vasi1186 CreditAttribution: vasi1186 commentedImplemented the changes from #24.
Comment #27
vasi1186 CreditAttribution: vasi1186 commentedPrevious patch generated some warnings.
Comment #28
vasi1186 CreditAttribution: vasi1186 commented#27: drupal-language_instance-1627208-27.patch queued for re-testing.
Comment #29
Gábor HojtsyExtra newline should not be there.
Why are you doing a list of languages here? It looks very, very odd.
New code looks much better than old code.
Comment #30
vasi1186 CreditAttribution: vasi1186 commentedFixed the issues reported in #29
Comment #31
Gábor HojtsyThis looks pretty strange to compare the object properties with an array. Can we instantiate a Language() instance for this instead?
Comment #32
vasi1186 CreditAttribution: vasi1186 commentedImplemented changes from #31
Comment #33
Gábor HojtsyCool, looks good now :) Thanks for working it all out.
Comment #34
Dries CreditAttribution: Dries commentedNice clean-up. Committed to 8.x. Thanks.
Comment #35
Gábor HojtsyThis should allow us to do nice things like type hinting for Language on functions, where that is required, things like:
Comment #36
RobLoachType-hinting would be a great addition! We'd probably want to have a
LanguageInterface
in #1512424: Add a LanguageInterface, and property setters/getters to Language class so that people are not limited to Language class implementations and people could provide their own Language classes if they wanted. Great idea, Gabor.Comment #37
vasi1186 CreditAttribution: vasi1186 commentedAgree with Rob. I think that every class in Drupal should implement an Interface and not force people to use only certain implementation of a class.
Comment #38
plachSwappable implementations have a cost, albeit not huge. I think we should not inconditionally put interfaces everywhere we have a class. Probably there are some cases where moving towards a slightly less flexible but more performant system would make sense.
That said I ain't sure about this particular case. Form the top of my head I cannot see the advantage of swapping the language implementation, but it might very well be lack of fantasy :)
Comment #39
vasi1186 CreditAttribution: vasi1186 commentedMaybe "every" class is a bit of an extreme case, that's true :)
But, if we talk about the Language class for example, let's say that somewhere we have to display a list of languages. In my opinion, the output of each Language should be handled by the class itself (for example a toString() like method). So, in this case I can imagine that people would want to display the Language differently in some applications. I know that this can be also done now using the theme system, but I just think that extending the Language class would be just a bit cleaner...
Comment #40
Gábor Hojtsy@vasi1186: hm, I don't think I get that use case.
Comment #41
vasi1186 CreditAttribution: vasi1186 commented@Gabor: for example if we have a block that lists all the available languages on the site (or in general, a list with languages). The output of each language (that usually is the language name, like English, German, etc..) could be overwritten in a class that would extend the Language class (for example a different display would be "English - en", "German - de", etc...)
Comment #42
Gábor HojtsyYeah, hm, I think overriding the language class for that sounds "a bit" hackish.
Comment #43
cosmicdreams CreditAttribution: cosmicdreams commentedThe strongest use case I can think of for having an interface for Language is tests. It may become easier to unit test all of language by providing a mock implementation of the Language Interface.
Comment #44
Gábor HojtsyWe should have a changelog entry for this as well as a change notice node. Anybody up for those?
Comment #45
RobLoachhttp://drupal.org/node/1647388
Comment #46
Gábor HojtsyReviewed and updated a bit. Looks good. Let's get into changelog too.
Comment #47
RobLoachLooks good with me! I'll reference the same changelog from #1512424: Add a LanguageInterface, and property setters/getters to Language class so we can update it accordingly once that's in.
Comment #48
Gábor HojtsyComment #49
RobLoach:-)
Comment #50
webchickCommitted and pushed to 8.x. Thanks!
Comment #51
webchickCommitted and pushed to 8.x. Thanks!
Comment #52
Gábor HojtsyComment #53
Gábor HojtsyRemoving sprint tag, #1512424: Add a LanguageInterface, and property setters/getters to Language class is where the action should be at.