Problem/Motivation
We have a LanguageInterface.
We should typehint on interfaces where possible.
Proposed resolution
Typehint using LanguageInterface instead of Language. There are several classes of changes that should be made:
- Constants: Constants should be called on the interface, not from the class. If you see something like 'Language::LANGCODE_DEFAULT', it should be: 'LanguageInterface::LANGCODE_DEFAULT'
- Properties: Properties only exist on the class, not on the interface. If you see something like 'Language::$defaultValues', this is correct. It should be the language class, not the languge inteface.
- New instance creation: If you're creating a new object instance, you do that on the class and not on the interface. If you see something like 'new Language(...)', it is correct. You instantiate classes, not interfaces.
- Unclassified usage: If you see something like 'use Drupal\Core\Language\Language;', use a tool like PhpStorm to check to see if the class is actually used. If you can show that it's not used, you *can* delete it, but it isn't required.
- Comments: You should make sure that any comments referring to the class or interface are referring to the appropriate object. You can tell which object is referred to by the context of the comment and the code it is referring to. (If you replace Language with LanguageInterface in a comment, please be sure to re-wrap the comment to wrap as closely to 80 characters as possible, to follow the Drupal code guidelines.) Pay special attention to @var, @param, @return, and @see parameters in comments, as they are used by Drupal even though they appear in comments.
Remaining tasks
- git instructions for creating patch | Contributor task documentation for creating a patch
- Review patch to check it fixes the issue, the change is properly documented and for coding standards. Make sure patch stays within scope of just this issue. | Contributor task documentaiton for reviewing patch
User interface changes
No
API changes
Yes.
Changing a typehint in a method changes how you can override it (minor API change depending how likely you are to subclass) so it is an API change. This probably will *not* break a lot of module code.
Related issues
- Split off from #2239497: [Meta] Fix ConfigurableLanguageManager getLanguages()
- #2271005: Rename Language module's LanguageInterface to ConfigurableLanguageInterface and Language to ConfigurableLanguage should help clarify things here and help for reviewing this. It is not a blocker here, but it would help.
- ?
Comment | File | Size | Author |
---|---|---|---|
#118 | 2246665-language_interface-118.patch | 293.33 KB | martin107 |
#115 | 2246665-language_interface-115.patch | 293.34 KB | martin107 |
#111 | 2246665-language_interface-111.patch | 293.29 KB | martin107 |
#111 | interdiff-110-111.txt | 645 bytes | martin107 |
#110 | 2246665-language_interface-110.patch | 292.82 KB | martin107 |
Comments
Comment #1
YesCT CreditAttribution: YesCT commentedfor a start, see some of the changes in patch #9 in #2239497: [Meta] Fix ConfigurableLanguageManager getLanguages() in core/lib/Drupal/Core/Language/LanguageDefault.php
Comment #2
tim.plunkettThere are actually two of these, so specifying which one. (I hope I picked the right one!)
Comment #3
YesCT CreditAttribution: YesCT commented:) #2246679: Make Language module's LanguageInterface (to be ConfigurableLanguageInterface) extend Core's LanguageInterface
Comment #4
Gábor HojtsyComment #5
tstoecklerThis updates all static constant usages in /core/includes, /core/lib and /core/tests, i.e. everything except for /core/modules.
This updates around 100 of 800 usages of the Language class in total, although a whole bunch of those are probably legitimate. Maybe we can commit this in batches? Don't know how long we have for the disruptive patch window, but would be nice to get this done.
Comment #7
tstoecklerForgot two missing use statements. This also updates a bunch more code.
Mostly the static constant usage in /core/modules is left.
Comment #9
tstoecklerHere we go. This fixes the fatal caused by using both LanguageInterface's in the same class. Aliased one to "BsaeLanguageInterface.
Also fixed modules up to and including config_translation.
Let's see what I broke this time.
Comment #11
tstoecklerHere we go.
Some more duplicate LanguageInterface's....
This fixes everything up to and including language module. This gets us down to just under 500 usages (again: of which many are legit).
Comment #13
YesCT CreditAttribution: YesCT commentedwait. you are taking out the un-used use statements. let's *not* do that here. #1989380: Remove unused "use" statements (eventually) will.
Comment #14
tstoecklerLet's see if this one is green.
Re #13: I'd really like to not revert those changes. Just like whitespace is spinach when reviewing patches, unused use statements are spinach when looking at the patch applied. Also they are trivial to review: in fact they can be completely ignored. Unlike unused variables, they actually cannot expose any bugs or unexpected behavior. Of course if you insist, I must defer, as strictly speaking those changes are unrelated, but yeah, I'd really rather not.
Comment #15
XanoWe have #2226533: Changes to the Language class due to the LanguageInterface (followup) as well and that one is older. Should this one be marked a duplicate?
Comment #16
XanoUgh, did not want to do that straight away.
Comment #17
filijonka CreditAttribution: filijonka commentedComment #18
filijonka CreditAttribution: filijonka commentedhopefully got this right
Comment #20
filijonka CreditAttribution: filijonka commentedreadded Database in the use statements on bootstrap..perhaps we should set back all..but let's see what happends and on the related issues too
Comment #22
filijonka CreditAttribution: filijonka commented14: 2246665-14-language-interface.patch queued for re-testing.
Comment #24
YesCT CreditAttribution: YesCT commented#14 was green when it ran. (it's red now cause it was retested but it doesn't apply)
kind of a strange git confusion/conflict in b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
I resolved it differently than @filijonka
and
one conflict was resolved keeping the patch line in
b/core/modules/content_translation/lib/Drupal/content_translation/Access/ContentTranslationManageAccessCheck.php
instead of keeping what was in head and just changing Language to LanguageInterface there.
#20 still applies so the interdiff here is to that to see how our rerolls were different.
let's see what the bot says about this one.
Comment #26
YesCT CreditAttribution: YesCT commentedyep. in irc @filijonka told me that would fail on bootstrap.
PHP Fatal error: Class 'Database' not found in /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/bootstrap.inc on line 409
I'm taking out the removal of non-language-related unused uses.
Comment #27
YesCT CreditAttribution: YesCT commented@tstoeckler in #11
So.. does that mean we should continue with changes in more places?
What strategy where you using?
Comment #28
filijonka CreditAttribution: filijonka commentedok so one approach to get this done is to divide each new try of a patch is to smaller pieces
first take care of everything in core/includes, then for each module in core/modules/
always making sure it's green in each step and after we done the above we do a new check for what is left?
any smarter suggestion?
Comment #29
tstoecklerRe @YesCT: I just opened the \Drupal\Core\Language\Language class and used the "Find usages" feature of PhpStorm (Alt+Shift+7 on Linux) and went through the resulting list from top to bottom.
I don't know how this should be done in terms of process. My initial plans were to finish this in time for the "disruptive patch window" after the next alpha (i.e. starting Wednesday this week). However, the fact that this is in part duplicated #2226533: Changes to the Language class due to the LanguageInterface (followup) by has de-motivated me to work on this issue.
I will try to ping @Xano on how to move these two issues forward in the most constructive way.
Comment #30
YesCT CreditAttribution: YesCT commentedI think getting this in for May 22 would be ideal, then we can reroll #2226533: Changes to the Language class due to the LanguageInterface (followup)
Comment #31
YesCT CreditAttribution: YesCT commentedok, 437 usages.
I'm a little iffy on how to tell if I can remove the use Language in files like the .inc ones.
left it in.
this is just locale.bulk.inc
Comment #32
YesCT CreditAttribution: YesCT commentedgoes up through and includes migrate.
was not sure if I could remove language/Language is couple places, so left those use's.
Comment #33
filijonka CreditAttribution: filijonka commentedreroll, everything went dandy..auto-mergin everything so nothing needed to be done
Comment #34
YesCT CreditAttribution: YesCT commented#2271005: Rename Language module's LanguageInterface to ConfigurableLanguageInterface and Language to ConfigurableLanguage
should help clarify things here and help for reviewing this.
not a blocker here, but it would help.
Comment #35
YesCT CreditAttribution: YesCT commentedComment #36
YesCT CreditAttribution: YesCT commentedclarifying the api change scope
Comment #37
YesCT CreditAttribution: YesCT commentedtalked with @filijonka in irc earlier about working on this, so unassigning it.
I'm going do some more now.
Comment #38
YesCT CreditAttribution: YesCT commentedyeah, that reroll looks great.
--
left the Language use in core/modules/node/node.module
(cause phpstorm didn't tell me it was an unused use)
also does
core/modules/node/node.tokens.inc
--
I'll do lib ones next, a bunch. I'm more confident in them than the .module or .inc ones.
Comment #39
YesCT CreditAttribution: YesCT commentedthese are the node/lib ones (and one core/modules/locale/lib/Drupal/locale/Tests/LocalePathTest.php it seems)
Comment #41
YesCT CreditAttribution: YesCT commentedthis reverts 3 of the removal of use Language that I was unsure of. phpstorm didn't grey them out, but I could not find where they were used in the class... hm.
Comment #43
YesCT CreditAttribution: YesCT commentedmight have been the revert of #2267753: ContentEntityDatabaseStorage::mapToStorageRecord hard-codes $entity->$name->value that this conflicted with.
rerolled for head.
diffing this patch and #40 shows only changes in the context lines.
Comment #45
YesCT CreditAttribution: YesCT commentederror was
Fatal error: Class 'Drupal\node\Tests\LanguageInterface' not found in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/node/lib/Drupal/node/Tests/NodeFieldMultilingualTestCase.php on line 104
FATAL Drupal\node\Tests\NodeFieldMultilingualTestCase: test runner returned a non-zero error code (255).
in #39 I changed the const typehint to languageinterface, but forgot to add the use.
adding that.
Comment #46
martin107 CreditAttribution: martin107 commentedOk so references to Language::SOMETHING have all been removed
this is just the first installment of a code tidy
that is removing all the extraneous noise that is unused "use" statement which will potentially be confusing to an IDE type hinting
by first installment ... I am about 25% of the way through my list!!!!!
Comment #47
martin107 CreditAttribution: martin107 commentedDouble postComment #49
martin107 CreditAttribution: martin107 commentedlocal testing shows this change gets this back to green ....
Fixing error before proceeding with further trimming
Comment #50
YesCT CreditAttribution: YesCT commentedComment #51
martin107 CreditAttribution: martin107 commentedFurther trimming ... I have advanced much further down my list of potential use statements to be removed
and found only 5 more ... so I no longer think a total of 4 trimming patches is unrealistic.
Comment #53
martin107 CreditAttribution: martin107 commentedDrat
Comment #54
martin107 CreditAttribution: martin107 commentedStraight reroll of #53.
PS As netbeans swaps from file to file it is quite happy to paste errors from the last active window into the next and 'update' in the background
hence all the false starts. ( So netbeans in unhelpful for this task!!!)
I am currently looking a getting phpcs to produce a report which I can grep.
Comment #55
martin107 CreditAttribution: martin107 commentedI can now count the number of unused use statements relating to the language system.
Explanation
I ran the phar file from https://github.com/fabpot/PHP-CS-Fixer
As I hope is obvious the php-cs-fixer command outputs a diff showing all the changes it would make to drupal ( this file is 28M so I won't paste it here )
This is then grep'ed to show all lines that are related to the language system and could be removed.
From patch #54 I count 16 which I will removed tomorrow in a single step
2 unused.txt !!! Only the small one is relvant
Comment #56
sunCan we pretty please defer the removal of unused use statements to the very very end of the release cycle? (as in: directly before tagging 8.0)
That's a one time operation. IDEs like phpStorm are able to perform it automatically with a single click. When doing that, we also want to sort all of them alphabetically. phpStorm can perform that automatically, too.
Let's just simply get this done here, please. Unused use statements don't harm anyone at all. Too much clean-up will break tons of other patches in the queue; let's not do that (now).
Comment #57
martin107 CreditAttribution: martin107 commentedMy apologies I will stop tinkering ... so going back to my comment in #46
I maybe misunderstood the remaining tasks in this issue
Can someone else review/RTBC?
Comment #58
YesCT CreditAttribution: YesCT commentedOh, I thought the @martin107 was removing the unused language use statements that *this* patch was making unused.
I agree that we do not want to make this patch any bigger than necessary at all.
So, if we take out a Language::Something
and replace it with LanguageInterface::Something
then if that makes Language unused, we should take out the use Language in that file
when we add the use LanguageInterface
Comment #59
YesCT CreditAttribution: YesCT commentedI'm re-rolling #45 now.
Comment #60
YesCT CreditAttribution: YesCT commentedrerolled. no interdiff. nothing tricky.
we should check where we change typehints in method definitions that the corresponding @param is updated also.
for example core/modules/config_translation/lib/Drupal/config_translation/Form/ConfigTranslationFormBase.php
Comment #61
YesCT CreditAttribution: YesCT commentedoops. attaching file.
Comment #63
martin107 CreditAttribution: martin107 commented@YesCT I was going to wake up in the morning and unwind my misconception in the same way.
Instead please find a minor tweak to #61 which locally makes ConfigTranslationUiTest pass.
Comment #64
YesCT CreditAttribution: YesCT commentedI'm going to work on this on the plane.
Comment #65
YesCT CreditAttribution: YesCT commentedfirst I just rerolled this. 2 easy conflicts to resolve, one in a use block and another from #2276183: Date intl support is broken, remove it
the "changes" patch is continuing the work after the reroll.
this goes through core/modules/taxonomy
I'll see what the bot says before I keep going.
Comment #68
martin107 CreditAttribution: martin107 commentedThe very next commit invalidated this "Mayfly" of a patch.
So Reroll.
locally MigrateCommentTest now passes
Comment #69
martin107 CreditAttribution: martin107 commentedComment #70
YesCT CreditAttribution: YesCT commentedthis is the last of those kind of changes.
now need to see if any other need to be done, and if this passes.
Comment #71
YesCT CreditAttribution: YesCT commentedreplaced in @var, @param, @return where appropriate (most places)
also went through the files and looked for un-used uses in files that were edited already by this patch.
Comment #72
filijonka CreditAttribution: filijonka commented71: 2246665-language_interface-71.patch queued for re-testing.
didn't apply locally but reroll went to easy so just making sure if it really needs a reroll.
Comment #74
filijonka CreditAttribution: filijonka commentedno conflicts on this, everything was automerging.
Comment #75
filijonka CreditAttribution: filijonka commentedwent through includes and lib in core.
Comment #76
filijonka CreditAttribution: filijonka commentedwent through core/modules, also checked everything else but couldn't find any more changes that should be done imho.
Would be very helpful to #2226533: Changes to the Language class due to the LanguageInterface (followup) to get this rtbc and committed.
Comment #78
jaredsmith CreditAttribution: jaredsmith commentedI'm going to working on this today during the Sunday sprint at DrupalCon Austin. Please grab me on IRC (jsmith) if you have comments/questions or want to help out.
Comment #79
filijonka CreditAttribution: filijonka commentedspelling correction..
Comment #80
jaredsmith CreditAttribution: jaredsmith commentedI've re-rolled the patch to better comply to the Drupal coding standards.
In particular:
* Make comments wrap as close to 80 columns as possible (see https://drupal.org/node/1354#drupal)
* Fix a couple of long arrays (see https://drupal.org/coding-standards#array)
I have not checked each individual change to make sure it makes logical sense, I have only focused on coding standards so far.
Comment #82
filijonka CreditAttribution: filijonka commentedThis is suddenly appearing in the #79 which is wrong. This is changed in #2256373: Factor HtmlFragment out to an interface so we need to get that correct in the patch. me, martin is on it and will get it fixed.
Comment #83
filijonka CreditAttribution: filijonka commented80: 2246665-language_interface-80.patch queued for re-testing.
Bot is green but we get a message that it failed so just sent a retest to see what happends.
Comment #84
jaredsmith CreditAttribution: jaredsmith commentedI've gone ahead and fixed up this patch to avoid the problems introduced in comment 79 (and explained in comment 82).
Here is what I've done:
Please let me know if you find anything else that needs to be done to move this issue forward.
(As a note, please don't freak out if the test for this patch gets canceled and re-submitted, as I'm working with jthorson to test out some new faster testbots, and using this issue as a great testing example.)
Comment #85
YesCT CreditAttribution: YesCT commentedthanks!
Comment #86
jaredsmith CreditAttribution: jaredsmith commentedI used PhpStorm to check all the usage of the Language class, and found a lot of use statements that are no longer used. I'll be around tomorrow morning (Austin time) to chat further about this patch.
Comment #88
jaredsmith CreditAttribution: jaredsmith commentedComment #89
jaredsmith CreditAttribution: jaredsmith commentedI have re-rolled this patch to keep it current with changes in HEAD, as the patches were no longer applying cleanly. I've also updated the issue summary with the list of items that needed to be done.
I've gone through and identified the 'use' statements that were no longer needed, and incorporated them into this patch as well. At this point, I think this patch covers all the items that need to be done. It handles the constants, the spurious 'use' statements, and the comments.
Comment #90
YesCT CreditAttribution: YesCT commentedOK I think we are close. We need reviews that help us know what needs work, or get us to rtbc.
Comment #92
filijonka CreditAttribution: filijonka commentedhi
is really removing use statements part of this issue?
Comment #93
jaredsmith CreditAttribution: jaredsmith commentedUgh, I totally messed up my patch in comment 89, and it didn't apply cleanly. Trying again.
Filijonka, there are certainly arguments for and against removing the "use" statements. I don't have a strong feeling one way or the other, but have leaned this direction for a couple of reasons. First, it made sense to remove then while I was reviewing each and every use of the Language class. Second, I think by having them as part of this patch, it makes it easier for the reviewer to review this patch, as they don't have to open every single file to find out which ones use the Language class. If the committers feel strongly about not removing already unused "use" constructs, I'd be happy to revert that portion of the change. I'm including the interdiff of just the "use" statement removal, to make it easier to remove them in the future if we wish.
Comment #95
martin107 CreditAttribution: martin107 commented@jsmith ... I caused a bit of a kerfuffle earlier removing unused use statements #56 was the response then..
Comment #96
filijonka CreditAttribution: filijonka commentedhi
ehm well removing use Language where we have been adding LanguageInterface is ok but I don't think we should mess with the rest of the use statement in this issue. e.g entity.inc.
DO NOT trust phpstorm blindly!
Recently I was involved in a issue where someone begun to remove use statement based on what phpstorm said. One of them was class Database and was removed. I'm using netbeans and there it wasn't marked as unused. The patch failed due to that Database wasn't actually unused. I've seen the other way around too so netbeans aren't any better so never fully trust the "automatic".
Comment #97
Gábor HojtsyLooks like for some reason the patch adds a
<default_langcode><value/></default_langcode>
into the serialization output, but does not change the test. Why add that to the serialization?Comment #98
filijonka CreditAttribution: filijonka commentedOk so after a discussion with @martin107 and looking through the discussion here we simply decided that patch in comment #84 is the current patch.
We are basing this on comment #56, #58 and #96.
Removing use statements made unused by changes we have made would be ok within this issue but not the rest.
Comment #99
filijonka CreditAttribution: filijonka commentedremoved this comment due it was misleading
Comment #100
Gábor HojtsyI don't agree the removal of use statements is cosmetic here. We want to educate people to use the interface instead of the class. If we leave references to using the class hanging around we give the wrong idea to developers. We want them to use the interface. #93 looks very easy to fix by removing the line accidentally crept in with the reroll.
Comment #101
filijonka CreditAttribution: filijonka commented@Gábor Hojtsy
Well I don't say that we shouldn't remove unused statement, I say that we shouldn't remove unused statement that we haven't created in this issue.
If we add a use statement making another one unsued we should remove that one. Im my opinion the latest patches from #84 is doing so much more than that.
And in #13 @yesct is refering to this issue #1989380: Remove unused "use" statements (eventually)
Comment #102
Gábor Hojtsy@filijonka #101: That does not address what I said in #100 about the developer education benefits of removing all the wrong uses of Language not just the ones changed by this patch.
Comment #103
jaredsmith CreditAttribution: jaredsmith commentedTrying my patch again, this time without the errant line identified in comment 97. Based on Gábor's notes in comment 100, I've left the removal of the "use" statements as part of this patch for the time being. (We can always take them by applying the interdiff-remove-use-statements.txt as a patch in reverse.)
Also, for the record, I didn't just blindly trust that PhpStorm knew whether the class was being used or not. I looked through each file where I removed the "use" statement to see if I could find anywhere that it was actually being used. (I hadn't used PhpStorm before yesterday, and frankly, didn't trust it. That being said, it's findings agreed with my own.)
For the ease of review, I'm also including a ".stats.txt" file, showing the diffstat of the patch, to make it easier to visualize the impact of this patch of various files within the Drupal core.
Comment #104
martin107 CreditAttribution: martin107 commentedI have given 2246665-language_interface-103.patch a slow and steady visual inspection.
It looks good so +1
Nothing out of scope, no stray code fragments --- all looks very good..
It really does clean a common misconception which is routed throughout the design.
I am sure there are instances in core which could be tweaked but this is a large improvement.
Given by it nature this task is brittle and likely to need reroll at any moment I would add mine to the other voices saying lets get this in quick and deal will any corner cases in followup.
Comment #105
Gábor HojtsyLooks good, let's get this in! Thanks all!
Comment #107
martin107 CreditAttribution: martin107 commentedReroll.
Comment #108
filijonka CreditAttribution: filijonka commentedper #105
Comment #110
martin107 CreditAttribution: martin107 commentedThis is just a straight reroll .. should we see green I have seen some more work to be done..
in the comments of language_list() bootstrap.inc -- constants need to be LanguageInterface::STATE_LOCKED
Comment #111
martin107 CreditAttribution: martin107 commentedFixed my nit-pick from #110
Comment #112
filijonka CreditAttribution: filijonka commentedgood martin107. problem with all these rerolls are that stuff seems to dissappear sometimes. I was sure we had done thos last changes too.
Comment #113
filijonka CreditAttribution: filijonka commentedComment #114
webchickSorry, needs one more re-roll after #1920862: Rename custom_block.module to block_content.module.
Comment #115
martin107 CreditAttribution: martin107 commented2 hours between rerolls ... this little issues feet really are made of clay.
Comment #117
martin107 CreditAttribution: martin107 commentedOk so while rerolling other commits caused further conflict .. its getting late .. I will try again in about 8 hours.
Comment #118
martin107 CreditAttribution: martin107 commentedreroll of #115 .. no conflicts just merging auto merging
Comment #119
filijonka CreditAttribution: filijonka commentedComment #120
webchickOk, let's get this in while it's hot!
Committed and pushed to 8.x. Thanks!
Comment #122
YesCT CreditAttribution: YesCT commentedwow! thanks for bringing this home!