Instead of LANGUAGE_TYPE_CONTENT, LANGUAGE_TYPE_URL, etc, we should have Language::CONTENT, Language::URL, etc. This will give us benefit of the autoloader, and reduce the amount of code in bootstrap.inc.
Postponed for:
- #1512308: Replace $language_content with Dependency Injection
- #1512310: Replace $language_url with Dependency Injection
- #1510686: Replace remaining references to global $language_interface with drupal_container()
Proposed change
<
Thanks! Should we prefix them with LANGUAGE_ even though they are under the Language class? Seems superfluous to me.
I totally agree with you.
Here is a list of constants the current patch moves, with a suggestion of a new naming. We should then probably move this to the issue summary.
Before | After |
LANGUAGE_SYSTEM | Language::LANGCODE_SYSTEM |
LANGUAGE_NOT_SPECIFIED | Language::LANGCODE_NOT_SPECIFIED |
LANGUAGE_NOT_APPLICABLE | Language::LANGCODE_NOT_APPLICABLE |
LANGUAGE_DEFAULT | Language::LANGCODE_DEFAULT |
LANGUAGE_CONFIGURABLE | Language::STATE_CONFIGURABLE |
LANGUAGE_LOCKED | Language::STATE_LOCKED |
LANGUAGE_ALL | Language::STATE_ALL |
LANGUAGE_SITE_DEFAULT | Language::STATE_SITE_DEFAULT |
LANGUAGE_TYPE_CONTENT | Language::TYPE_CONTENT |
LANGUAGE_TYPE_INTERFACE | Language::TYPE_INTERFACE |
LANGUAGE_TYPE_URL | Language::TYPE_URL |
LANGUAGE_LTR | Language::DIRECTION_LTR |
LANGUAGE_RTL | Language::DIRECTION_RTL |
Comment | File | Size | Author |
---|---|---|---|
#86 | drupal-1620010-86.patch | 419.01 KB | dawehner |
#86 | interdiff.txt | 510 bytes | dawehner |
#84 | drupal-1620010-84.patch | 418.87 KB | ParisLiakos |
#84 | interdiff.txt | 1.97 KB | ParisLiakos |
#77 | drupal-1620010-76.patch | 418.46 KB | jibran |
Comments
Comment #1
plachPerhaps the variables should be better "scoped": what about
Language::TYPE_(INTERFACE|CONTENT|URL)
?Comment #2
plachI mean this would leave room for migrating other constants that now live in boostrap.inc. I'm thinking about
Language::NOT_SPECIFIED
,Language::NOT_APPLICABLE
and so on. We probably want to that here as well.Comment #3
RobLoachTagging.
Comment #4
Gábor HojtsyI think the main question still is what do we want to do with #1512424: Add a LanguageInterface, and property setters/getters to Language class? If you language_load('de'), you *don't* get an instance of the language class. See http://drupal.org/node/1512424#comment-6101938 - I think this would make sense if we use the Language class more globally. Currently, its limited to the DI / negotiation layer only.
Comment #5
RobLoachHmm, seems like there are more places where we could take advantage of this :-) . Do you think having
language_list()
andlanguage_load()
return Language objects should be in this done in this issue, or a separate one?Comment #6
Gábor HojtsyOpened #1627208: Make language_list() and language_default() return instances of Language for that.
Comment #7
Gábor HojtsyAll of those landed, so looks like this can now be started, right?
Comment #8
Gábor HojtsyMoving off the D8MI focus board.
Comment #9
dawehnerJust started to move the constants to the language class, but left out most of LANGUAGE_NOT_SPECIFIED and LANGUAGE_DEFAULT.
Renaming is easy if you have all use statements in place.
Comment #10
Gábor HojtsyThanks! Should we prefix them with LANGUAGE_ even though they are under the Language class? Seems superfluous to me. Also, different language constants have very different meanings. Eg. LANGUAGE_TYPE_* are language negotiation types, not types of languages :)
Comment #11
plachFor those I'd go with #1.
Comment #12
dawehner<
I totally agree with you.
Here is a list of constants the current patch moves, with a suggestion of a new naming. We should then probably move this to the issue summary.
Comment #13
dawehnerReally not sure whether it's worth to convert that now, as it will certainly conflict with all the EntityNG changes.
Comment #14
alexpottLet's see what the testbot thinks
Comment #16
plach@dawehner:
Overall the proposal looks good to me, but:
These could become simpler:
Language::CODE_SYSTEM
.These sould really be prefixed with TYPE: we spent a lot of time discussing and standardizing on a terminology:
Language::TYPE_CONTENT
.Comment #17
Gábor HojtsyAgreed with @plach on TYPE, not on CODE :) We also spent a lot of time standardising on "langcode" vs. anything else :D
Comment #18
plachRight :)
Comment #18.0
plachUpdated issue summary.
Comment #19
dawehnerThank you for the feedback!
Added the feedback from #16 and #17 to the issue summary and updated the patch.
Seriously, I need a better internet connection for these kind of patches :)
Comment #20
plachThis fixes some wrong constant names and rewraps comments to column 80. The interdiff covers only the latter.
Comment #22
dawehnerFixes hopefully all problems.
Comment #23
fagoLANGUAGE_DEFAULT is right now not used as constant for language_default() but for the default language of an entity. Still, we could keep the name and use it to refer to the default language in whatever context? Or is that confusing and we should rename it?
Comment #24
Gábor Hojtsy@fago: not sure how can LANGUAGE_DEFAULT ('und') accurate to signify defaultness at places where the default might be different. Eg. content types, vocabularies, etc. have a setting of default language for new entities created, and one possible option is 'und', while there are other options. So the default language of those entities when created is not necessarily 'und', however that is one of the possible options as well, so I'm not seeing how it can be used generally as-is. Not sure what it was meant to do, I believe it was introduced with the "property api" patches.
Comment #25
plach@fago:
Do you mean using a
LANGUAGE_DEFAULT
constant instead oflanguage_default()
?Comment #26
fagoIt's used to refer to the entity default language - what ever it is. It's value is 'und' right now because of the BC-mode, however a new, better BC-mode just got committed. I think with the new BC mode changing the constant should work now - thus we could move it over to a more reasonable value.
No, language_default() is getting the site's default language. I'd use the constant to refer to the language default depending on the context, e.g. in site context it's language_default() while in entity context it's $entity->langcode->value.
Comment #28
fagoGave it a short test - no unfortunately the new BC-mode does not allow to change the constant right now either. We'll need to wait for the BC mode to be removed for changing it.
I also noted we already have a site default language constant. Should we just make it an entity default language constant?
Comment #29
plachOk, now I get it :)
Not sure about this, I think we should try and see how it looks while addressing the active language/active translation issue. See for instance #1807692-13: Introduce a column synchronization capability and use it to translate alt and titles through the image field widget (
EntityTranslationController.php
and related hunks): the current solution is a hack, we should try to find a better one. For instance passing along anEntityTranslation
object instead of anEntity
object. Anyway we'll need an issue to dicuss this.Comment #30
dawehnerJust another rerole.
Comment #32
dawehnerComment #34
dawehner.
Comment #35
dawehnerAnother rerole, hopefully I catched all of them.
Comment #37
dawehnerLess fails, yeah!
Comment #38
dawehnerlinebreak.
Let's keep this small fix out of the patch.
dito
Damn, this was probably part of the merge conflict.
linebreak.
linebreak.
Indentation.
Comment #40
dawehnerIt seems to make sense to skip the line breaks in this issue, as they have the potential to blow up this patch even more. Would do you think?
This patch should be really green now.
Comment #42
dawehnerComment #44
dawehnerI can reproduce the error locally, it's something quite odd: it tells you that the ConfigStorageController uses the LANGUAGE_NOT_SPECIFIED constant, even this constant is not used anywhere.
My theory is that this const is somewhere stored in APC and then PHP get's confused. This one is based upon the experimental behavior of going away when restarting apache.
Comment #45
msonnabaum CreditAttribution: msonnabaum commented#42: drupal-1620010-42.patch queued for re-testing.
Comment #46
ParisLiakos CreditAttribution: ParisLiakos commenteddreditor crashed on me:/
anyways, good job, i just found that several comments now exceed 80 chars
Comment #47
dawehnerWrapped the 80 chars.
Comment #49
dawehnerRerolled against a views change.
Comment #50
ParisLiakos CreditAttribution: ParisLiakos commentedok, went through most of it again, looks legit, i grep'ed, couldnt find anything forgotten
good job!
i hope bot agrees and you dont reroll this many times till commit, although i doubt:/
Comment #51
ParisLiakos CreditAttribution: ParisLiakos commentedsorry, my grep was overengineered:P
found some left
Comment #52
dawehnerAren't all of those not comments, so 80 chars is not an issue?
Comment #53
dawehnerBack to needs review.
Comment #54
ParisLiakos CreditAttribution: ParisLiakos commentedtestbot failed..also i dont talk about wraps:)
$this->assertTrue(t($name, array(), array('langcode' => Language::LANGCODE_SYSTEM)) == $name, t('t() works for Language::LANGUAGE_SYSTEM.'));
should be
$this->assertTrue(t($name, array(), array('langcode' => Language::LANGCODE_SYSTEM)) == $name, t('t() works for Language::LANGCODE_SYSTEM.'));
or
./core/modules/node/lib/Drupal/node/Tests/PagePreviewTest.php: 'langcode' => LANGUAGE_NOT_SPECIFIED,
should be
Language::LANGCODE_NOT_SPECIFIED
Comment #55
dawehnerI seriously hope we will get this in sooner or later.
git add -p isn't fun on that patch :)
Comment #57
Gábor HojtsyWe will definitely needs avoid commit conflicts on this.
Comment #58
dawehnerI fear that this requires a LOT of other patches to be rerolled, so rerolling just this over time, seems better for the total community.
Comment #59
plachYep, also because we may want to move some constants to the language manager as proposed in #1862202: Objectify the language system.
Comment #60
dawehner@plach
Do you have a prefered order of getting it in? I'm fine with rerolling both of it.
Comment #61
plachWell, I think it depends on when I'm able to get back to that issue. If this is ready and there are no big RTBC patches around I guess we can simply go on and reroll #1862202: Objectify the language system afterwards. Or we can simply move the proper constants that to the language manager directly here (don't remember exactly which ones).
Comment #62
Gábor Hojtsy@dawehner: agreed with this, removing conflict avoidance tag.
Comment #63
alexpottWeird... tag was not actually removed
Comment #64
ParisLiakos CreditAttribution: ParisLiakos commentedtagging with needs reroll
Comment #65
dawehner@plach, @gabor
If you think it's time for that patch, just comment here, and I will do another rerole.
Comment #66
Berdir#58: drupal-1620010-58.patch queued for re-testing.
Comment #68
msonnabaum CreditAttribution: msonnabaum commentedDoesn't apply anymore, but if a reroll passes, I will RTBC this so hard.
Comment #69
dawehnerComment #71
RobLoachAs long as this passes, it's RTBC.
Comment #72
dawehnerRemoved the unused statements in access controllers.
Comment #74
ParisLiakos CreditAttribution: ParisLiakos commented#72: drupal-1620010-72.patch queued for re-testing.
Comment #76
ParisLiakos CreditAttribution: ParisLiakos commentedfor your rtbc pleasure
Comment #77
jibran#72 missed
LANGUAGE_NOT_APPLICABLE
inbootstrap.inc
Comment #79
jibranCrosspost. RTBC as per #68 and #71
Comment #80
aspilicious CreditAttribution: aspilicious commented#77: drupal-1620010-76.patch queued for re-testing.
Comment #82
plach#77: drupal-1620010-76.patch queued for re-testing.
Comment #84
ParisLiakos CreditAttribution: ParisLiakos commenteda forgotten use statement, and well a class named Language:)
Comment #86
dawehnerArg
Comment #87
tim.plunkettRTBC if green.
Comment #88
alexpottCommitted f2d710c and pushed to 8.x. Thanks!
Comment #89
Gábor HojtsyAdded change notice at https://drupal.org/node/2011418
Comment #90
plachAwesome :)
Comment #91.0
(not verified) CreditAttribution: commentedadded proposed