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.
From 8.x-3.x.
Comment | File | Size | Author |
---|---|---|---|
#25 | drupal-1828528-25.patch | 15.45 KB | dawehner |
#15 | interdiff.txt | 1.03 KB | tim.plunkett |
#15 | vdc-1828528-15.patch | 20.63 KB | tim.plunkett |
#9 | interdiff.txt | 645 bytes | dawehner |
#9 | drupal-1828528-9.patch | 20.62 KB | dawehner |
Comments
Comment #1
xjmSwitch to "contains".
Verbs.
These class names could use some work. See #1809930: [META] Many core class names violate naming standards. I'd prefer to just name them correctly here. They'd be different from the handlers already in core, but IMO that's fine.
These docblocks are nonstandard.
Need docblocks.
Check on the status of this.
Capitalize "English".
Comment #2
xjmComment #3
xjmComment #4
dawehnerThe changes are looking perfect and i think for various reasons it is important (as there are thrown debug messages on a lot of tests).
Comment #5
xjmLet's make sure this does actually pass the bot before we commit it since I renamed a bunch of classes. Also I'd like Gábor to take a glance at it to make sure this still makes sense in D8.
Comment #6
Gábor HojtsyThe patch generally looks good. I caught this thing though which should be solved:
I'm not sure about this. Drupal 8 certainly has 3 special languages, not 1 only. Also language_list() (as seem to be used in views_language_list()) will not return the locked languages, so you might want to be add it back again here.
But the reason of them being returned by language_list() is that users can reorder them (and/or modules can add more). So relying on that would be Drupal 8 compatible. As-is this right now it does not use the two new special languages and mischaracterizes the not specified language.
Comment #7
dawehnerWell we do rely on that already with some really funny code :)
This patches fixes that function and sets LANGUAGE_ALL as default behavior, because views is using that most of the time.
Comment #8
Gábor HojtsyLooks like a good improvement to me. Speaking of which, seems like the view data does not inclue the locked column for languages, so you cannot filter for languages like that, etc. I think it would be pretty useful, think listing content in known languages vs. special/unknown languages separately on an admin overview, building a view with content only in known languages, etc.
Comment #9
dawehnerThanks for you feedback!
Would this helper method be useful for language.module itself?
Added the locked column.
Comment #10
Gábor HojtsyWell, there used to be a helper like that in D7 I think and it was removed because it was not used much and was confusing to have multiple language list functions.
Comment #11
dawehnerOkay then it is probably fine to have it just in views. Would you think, with this locking enabled that everything is setup probably?
Comment #12
Gábor HojtsyLooks good to me, found no other issues.
Comment #13
Gábor HojtsyComment #15
tim.plunkettRerolled for the protected properties change.
Comment #17
dawehner#15: vdc-1828528-15.patch queued for re-testing.
Comment #19
webflo CreditAttribution: webflo commented#15: vdc-1828528-15.patch queued for re-testing.
Comment #21
xjm#15: vdc-1828528-15.patch queued for re-testing.
Comment #23
dawehner#15: vdc-1828528-15.patch queued for re-testing.
Comment #25
dawehnerLet's
This is actually part of another issue, so let's skip those files.
Comment #26
Gábor HojtsyLooks good to me now :)
Comment #27
catchI've been assuming that languages will at some point become configuration entities but can't find the issue. If we made that change though then most of the code added here would be removed again. I don't mind committing it then removing it later but it'd be good to know what the eventual plan is.
Comment #28
xjm#1754246: Languages should be configuration entities is that issue.
I'd prefer to put the integration in now, since apparently various other things are blocked on it, and it adds test coverage which will be useful in the other issue.
Comment #29
dawehnerThere are basically two parts of the patch.
Removing the first part later is easy but the second part is actually helpful, because for example node.views.inc assumes it is there
already.
Comment #30
catchOK fair enough. Committed/pushed to 8.x.
Comment #32
xjm