Typehint with Drupal\Core\Language\LanguageInterface instead Drupal\Core\Language\Language

There are actually two of these, so specifying which one. (I hope I picked the right one!)

This 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.

Forgot two missing use statements. This also updates a bunch more code.

Mostly the static constant usage in /core/modules is left.

Here 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.

Here 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).

Let'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.

We 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?

Ugh, did not want to do that straight away.

hopefully got this right

readded 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

14: 2246665-14-language-interface.patch queued for re-testing.

ok 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?

Re @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.

reroll, everything went dandy..auto-mergin everything so nothing needed to be done

Ok 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!!!!!

Double post

local testing shows this change gets this back to green ....

Fixing error before proceeding with further trimming

Further 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.

Drat

Straight 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.

I 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

php php-cs-fixer.phar fix --fixers=unused_use --dry-run --verbose --diff . | grep '\-use Drupal\\Core\\Language\\' > unused.txt

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

Can 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).

My apologies I will stop tinkering ... so going back to my comment in #46

Ok so references to Language::SOMETHING have all been removed

I maybe misunderstood the remaining tasks in this issue

Can someone else review/RTBC?

@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.

The very next commit invalidated this "Mayfly" of a patch.

So Reroll.

locally MigrateCommentTest now passes

71: 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.

no conflicts on this, everything was automerging.

went through includes and lib in core.

went 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.

I'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.

spelling correction..

I'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.

+++ b/core/lib/Drupal/Core/Page/HtmlFragmentRendererInterface.php
@@ -33,6 +33,6 @@
-  public function render(HtmlFragmentInterface $fragment, $status_code = 200);
+  public function render(HtmlFragment $fragment, $status_code = 200);

This 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.

80: 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.

I'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:

• Checked out the latest code from the 8.x branch
• Applied the patch from 76
• Applied the interdiff from 76-78
• Applied the interdiff from 78-80

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.)

I 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.

I 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.

hi

is really removing use statements part of this issue?

Ugh, 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.

@jsmith ... I caused a bit of a kerfuffle earlier removing unused use statements #56 was the response then..

hi

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".

Looks 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?

Ok 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.

removed this comment due it was misleading

I 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.

@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)

@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.

Trying 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.

I 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.

Looks good, let's get this in! Thanks all!

Reroll.

per #105

This 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

/**
 * Returns a list of languages set up on the site.
 *
 * @param $flags
 *   (optional) Specifies the state of the languages that have to be returned.
 *   It can be: Language::STATE_CONFIGURABLE, Language::STATE_LOCKED,
 *   Language::STATE_ALL.
 *
 * @return array
 *   An associative array of languages, keyed by the language code, ordered by
 *   weight ascending and name ascending.
 *
 * @deprecated in Drupal 8.x-dev, will be removed before Drupal 8.0.
 *   Use \Drupal::languageManager()->getLanguages().
 */
function language_list($flags = LanguageInterface::STATE_CONFIGURABLE) {
  return \Drupal::languageManager()->getLanguages($flags);
}

Fixed my nit-pick from #110

good martin107. problem with all these rerolls are that stuff seems to dissappear sometimes. I was sure we had done thos last changes too.

Sorry, needs one more re-roll after #1920862: Rename custom_block.module to block_content.module.

2 hours between rerolls ... this little issues feet really are made of clay.

Ok so while rerolling other commits caused further conflict .. its getting late .. I will try again in about 8 hours.

reroll of #115 .. no conflicts just merging auto merging

Ok, let's get this in while it's hot!

Committed and pushed to 8.x. Thanks!