Part of meta #1853522: [META] (Re)introduce Views data integration for core modules. From the 8.x-3.x branch of Views.
Comment | File | Size | Author |
---|---|---|---|
#10 | interdiff.txt | 10.03 KB | jibran |
#10 | 1853534-10.patch | 14.79 KB | jibran |
#7 | 1853534-7.patch | 14.35 KB | jibran |
#7 | interdiff.txt | 1.61 KB | jibran |
#5 | 1853534-5.patch | 14.34 KB | jibran |
Comments
Comment #2
dawehnerWow the schema of locale really changed a lot since the last time someone had a look at it.
Should we actually also apply the full codestyles to new/reintroduced files?
The handlers all patch is simply great as it automatically tests that you don't have fields which aren't in the database anymore.
Comment #3
aspilicious CreditAttribution: aspilicious commentedAll those functions need a docblock. Something like "Overrides ..." or "Implements ..."
Same
A basic view that uses this intgeration would increase our test coverage a lot.
Comment #4
dawehnerThis just fixes the comments, we didn't came to a good conclusion regarding the test coverage, but i agree at least the custom handlers should be tested.
Comment #5
jibranReroll. Do we need test for this?
Comment #7
jibranDid you maybe forget to add a "use" statement for this annotation? Yes
Comment #8
dawehnerThanks for taking this up!!!!
Should be Contains ...
Needs an @inheritdoc
You could also inject the database connection into the handler.
It doesn't provide handlers anymore.
Let's drop all this pointless comments.
With drupal 8 all this is click sortable by default, so we can remove those.
Comment #10
jibranThanks @dawehner for the great review. Fixed all #8 and failure in #7.
Comment #11
dawehnerSadly the current version of the test does not contain any kind of test :(
Comment #12
jibranComment #13
xjmComment #14
xjmComment #15
xjmComment #16
Gábor Hojtsy@jibran pinged me about this in IRC. I think this would still be great to include for custom admin pages. However the locale tables changed significantly since then, so the code would need to be updated significantly.
What does translatable mean here?
Should only be English in Drupal 8. Non-English sources are never maintained in locale, at least definitely not by core and not intended for contrib either.
Not true. This is the last version the string was used with. See locale.install for a more accurate description.
These columns do not exist anymore, however there is a "Customized" column which would be important to expose.
What's sid in locales_location is lid in both locales_source and locales_target. (I know the DB should have been cleaned up, but we did not get around to that). It is 100% sure that joining from lid to lid will not be good because they mean different things in the two tables.
Locale file instead of group IMHO.
Also lots of columns missing from this integration (project, version, last_checked).
Seems to be a nonexistent table at this point. Most columns integrated to locale_file, see above.
Don't think this one is needed either given the above notes about the table being removed.
Also definitely needs a test view or two.
Comment #17
xjmThis issue was marked as a beta target for the 8.0.x beta, but is not applicable as an 8.1.x beta target, so untagging.
As a feature addition, it should be targeted for 8.2.x at this point.
Comment #32
LendudeDiscussed with @Gábor Hojtsy at ddd23, we agreed, this is still worth adding.