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

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

CommentFileSizeAuthor
#118 2246665-language_interface-118.patch293.33 KBmartin107
#115 2246665-language_interface-115.patch293.34 KBmartin107
#111 2246665-language_interface-111.patch293.29 KBmartin107
#111 interdiff-110-111.txt645 bytesmartin107
#110 2246665-language_interface-110.patch292.82 KBmartin107
#107 2246665-language_interface-107.patch292.81 KBmartin107
#103 interdiff-2246665-93-103.txt1.51 KBjaredsmith
#103 2246665-language_interface-103.stats_.txt14.58 KBjaredsmith
#103 2246665-language_interface-103.patch292.91 KBjaredsmith
#93 interdiff-remove-use-statements.txt32.83 KBjaredsmith
#93 2246665-language_interface-93.patch292.98 KBjaredsmith
#89 2246665-language_interface-87.patch293.04 KBjaredsmith
#86 2246665-language_interface-86.patch293.04 KBjaredsmith
#86 interdiff-2246665-84-86.txt32.82 KBjaredsmith
#84 2246665-language_interface-84.patch261.87 KBjaredsmith
#80 2246665-language_interface-80.patch269.19 KBjaredsmith
#80 interdiff-2246665-78-80.txt8.02 KBjaredsmith
#79 2246665-language_interface-78.patch267.54 KBfilijonka
#79 interdiff-2246665-76-78.txt820 bytesfilijonka
#76 2246665-language_interface-76.patch260.13 KBfilijonka
#76 interdiff-2246665-75-76.txt16.4 KBfilijonka
#75 2246665-language_interface-75.patch249.15 KBfilijonka
#75 interdiff-2246665-74-75.txt9.79 KBfilijonka
#74 2246665-language_interface-74.patch241.76 KBfilijonka
#71 interdiff-2246665-70-71.txt13.04 KBYesCT
#71 2246665-language_interface-71.patch242.63 KBYesCT
#70 interdiff-2246665-68-70.txt13.12 KBYesCT
#70 2246665-language_interface-70.patch234.62 KBYesCT
#68 2246665-language_interface-68-thechanges.patch221.49 KBmartin107
#65 interdiff-2246665-65reroll-65changes.txt59.54 KBYesCT
#65 2246665-language_interface-65-thechanges.patch221.49 KBYesCT
#65 2246665-language_interface-65-thereroll.patch162.22 KBYesCT
#63 interdiff-61-63.txt800 bytesmartin107
#63 2246665-language_interface-63.patch166.97 KBmartin107
#61 2246665-language_interface-60.patch167.03 KBYesCT
#55 unused.txt727 bytesmartin107
#55 unused.txt31.78 KBmartin107
#54 2246665-language_interface-54.patch185 KBmartin107
#53 2246665-language_interface-53.patch185.01 KBmartin107
#53 interdiff-51.53.txt971 bytesmartin107
#51 interdiff-49-51.txt2.93 KBmartin107
#51 2246665-language_interface-51.patch185.48 KBmartin107
#49 interdiff-46-49.txt1021 bytesmartin107
#49 2246665-language_interface-49.patch183.03 KBmartin107
#46 interdiff-45-46.txt16.49 KBmartin107
#46 2246665-language_interface-46.patch183.44 KBmartin107
#45 interdiff-2246665-43-45.txt612 bytesYesCT
#45 2246665-language_interface-45.patch166.95 KBYesCT
#43 2246665-language_interface-43.patch166.69 KBYesCT
#41 interdiff-2246665-39-40.txt1.65 KBYesCT
#41 2246665-language_interface-40.patch166.65 KBYesCT
#39 interdiff-2246665-38-39.txt10.58 KBYesCT
#39 2246665-language_interface-39.patch166.78 KBYesCT
#38 interdiff-2246665-33-38.txt1.54 KBYesCT
#38 2246665-38-language-interface.patch156.67 KBYesCT
#33 2246665-33.patch154.48 KBfilijonka
#32 interdiff-2246665-31-32.txt14.77 KBYesCT
#32 2246665-32-language-interface.patch155.12 KBYesCT
#31 interdiff-2246665-26-31.txt1.07 KBYesCT
#31 2246665-31-language-interface.patch140.36 KBYesCT
#26 interdiff-2246665-24-26.txt9 KBYesCT
#26 2246665-26-language-interface.patch139.29 KBYesCT
#24 interdiff-2246665-20-24.txt7.25 KBYesCT
#24 2246665-24-language-interface.patch142.22 KBYesCT
#20 2246665-20.patch141.2 KBfilijonka
#18 2246665-18.patch141.38 KBfilijonka
#18 interdiff-2246665-14-18.txt10.25 KBfilijonka
#14 2246665-14-language-interface.patch142.29 KBtstoeckler
#14 2246665-11-14-interdiff.txt1.35 KBtstoeckler
#11 2246665-9-11-interdiff.txt2.39 KBtstoeckler
#11 2246665-11-language-interface.patch142.66 KBtstoeckler
#9 2246665-9-language-interface.patch133.62 KBtstoeckler
#9 2246665-7-9-interdiff.txt13.28 KBtstoeckler
#7 2246665-7-language-interface.patch121.26 KBtstoeckler
#7 2246665-5-7-interdiff.txt29.85 KBtstoeckler
#5 2246665-5-language-interface.patch47.87 KBtstoeckler
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

YesCT’s picture

for a start, see some of the changes in patch #9 in #2239497: [Meta] Fix ConfigurableLanguageManager getLanguages() in core/lib/Drupal/Core/Language/LanguageDefault.php

tim.plunkett’s picture

Title: Typehint LanguageInterface instead Language » 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!)

Gábor Hojtsy’s picture

tstoeckler’s picture

Status: Active » Needs review
FileSize
47.87 KB

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.

Status: Needs review » Needs work

The last submitted patch, 5: 2246665-5-language-interface.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
29.85 KB
121.26 KB

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

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

Status: Needs review » Needs work

The last submitted patch, 7: 2246665-7-language-interface.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
13.28 KB
133.62 KB

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.

Status: Needs review » Needs work

The last submitted patch, 9: 2246665-9-language-interface.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
142.66 KB
2.39 KB

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

Status: Needs review » Needs work

The last submitted patch, 11: 2246665-11-language-interface.patch, failed testing.

YesCT’s picture

wait. you are taking out the un-used use statements. let's *not* do that here. #1989380: Remove unused "use" statements (eventually) will.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
1.35 KB
142.29 KB

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.

Xano’s picture

Status: Needs review » Closed (duplicate)

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?

Xano’s picture

Status: Closed (duplicate) » Needs review

Ugh, did not want to do that straight away.

filijonka’s picture

Assigned: Unassigned » filijonka
filijonka’s picture

FileSize
10.25 KB
141.38 KB

hopefully got this right

Status: Needs review » Needs work

The last submitted patch, 18: 2246665-18.patch, failed testing.

filijonka’s picture

Status: Needs work » Needs review
FileSize
141.2 KB

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

Status: Needs review » Needs work

The last submitted patch, 20: 2246665-20.patch, failed testing.

filijonka’s picture

The last submitted patch, 14: 2246665-14-language-interface.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review
FileSize
142.22 KB
7.25 KB

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

Status: Needs review » Needs work

The last submitted patch, 24: 2246665-24-language-interface.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review
FileSize
139.29 KB
9 KB

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

YesCT’s picture

@tstoeckler in #11

This fixes everything up to and including language module. This gets us down to just under 500 usages (again: of which many are legit).

So.. does that mean we should continue with changes in more places?
What strategy where you using?

filijonka’s picture

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?

tstoeckler’s picture

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.

YesCT’s picture

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

YesCT’s picture

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

YesCT’s picture

goes up through and includes migrate.

was not sure if I could remove language/Language is couple places, so left those use's.

filijonka’s picture

FileSize
154.48 KB

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

YesCT’s picture

YesCT’s picture

Issue summary: View changes
Issue tags: +API change
YesCT’s picture

Issue summary: View changes

clarifying the api change scope

YesCT’s picture

Assigned: filijonka » Unassigned

talked with @filijonka in irc earlier about working on this, so unassigning it.
I'm going do some more now.

YesCT’s picture

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

YesCT’s picture

these are the node/lib ones (and one core/modules/locale/lib/Drupal/locale/Tests/LocalePathTest.php it seems)

Status: Needs review » Needs work

The last submitted patch, 39: 2246665-language_interface-39.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review
FileSize
166.65 KB
1.65 KB

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

Status: Needs review » Needs work

The last submitted patch, 41: 2246665-language_interface-40.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review
FileSize
166.69 KB

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

Status: Needs review » Needs work

The last submitted patch, 43: 2246665-language_interface-43.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review
FileSize
166.95 KB
612 bytes

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

martin107’s picture

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

martin107’s picture

Double post

Status: Needs review » Needs work

The last submitted patch, 46: 2246665-language_interface-46.patch, failed testing.

martin107’s picture

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

Fixing error before proceeding with further trimming

YesCT’s picture

Status: Needs work » Needs review
martin107’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 51: 2246665-language_interface-51.patch, failed testing.

martin107’s picture

Status: Needs work » Needs review
FileSize
971 bytes
185.01 KB

Drat

martin107’s picture

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.

martin107’s picture

FileSize
31.78 KB
727 bytes

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

sun’s picture

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

martin107’s picture

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’s picture

Oh, 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

YesCT’s picture

I'm re-rolling #45 now.

YesCT’s picture

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

YesCT’s picture

oops. attaching file.

Status: Needs review » Needs work

The last submitted patch, 61: 2246665-language_interface-60.patch, failed testing.

martin107’s picture

Status: Needs work » Needs review
FileSize
166.97 KB
800 bytes

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

YesCT’s picture

I'm going to work on this on the plane.

YesCT’s picture

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

The last submitted patch, 65: 2246665-language_interface-65-thereroll.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 65: 2246665-language_interface-65-thechanges.patch, failed testing.

martin107’s picture

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

So Reroll.

locally MigrateCommentTest now passes

martin107’s picture

Status: Needs work » Needs review
YesCT’s picture

this is the last of those kind of changes.
now need to see if any other need to be done, and if this passes.

YesCT’s picture

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

filijonka’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 71: 2246665-language_interface-71.patch, failed testing.

filijonka’s picture

Status: Needs work » Needs review
FileSize
241.76 KB

no conflicts on this, everything was automerging.

filijonka’s picture

went through includes and lib in core.

filijonka’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 76: 2246665-language_interface-76.patch, failed testing.

jaredsmith’s picture

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.

filijonka’s picture

Status: Needs work » Needs review
FileSize
820 bytes
267.54 KB

spelling correction..

jaredsmith’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 80: 2246665-language_interface-80.patch, failed testing.

filijonka’s picture

Assigned: Unassigned » filijonka
+++ 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.

filijonka’s picture

Status: Needs work » Needs review

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.

jaredsmith’s picture

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

YesCT’s picture

thanks!

jaredsmith’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 86: 2246665-language_interface-86.patch, failed testing.

jaredsmith’s picture

Issue summary: View changes
jaredsmith’s picture

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.

YesCT’s picture

Assigned: filijonka » Unassigned
Status: Needs work » Needs review

OK I think we are close. We need reviews that help us know what needs work, or get us to rtbc.

Status: Needs review » Needs work

The last submitted patch, 89: 2246665-language_interface-87.patch, failed testing.

filijonka’s picture

hi

is really removing use statements part of this issue?

jaredsmith’s picture

Component: language.module » language system
Status: Needs work » Needs review
FileSize
292.98 KB
32.83 KB

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.

Status: Needs review » Needs work

The last submitted patch, 93: 2246665-language_interface-93.patch, failed testing.

martin107’s picture

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

filijonka’s picture

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

Gábor Hojtsy’s picture

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?

filijonka’s picture

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.

filijonka’s picture

removed this comment due it was misleading

Gábor Hojtsy’s picture

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.

filijonka’s picture

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

Gábor Hojtsy’s picture

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

jaredsmith’s picture

Status: Needs work » Needs review
FileSize
292.91 KB
14.58 KB
1.51 KB

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.

martin107’s picture

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.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +sprint

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 103: 2246665-language_interface-103.patch, failed testing.

martin107’s picture

Status: Needs work » Needs review
FileSize
292.81 KB

Reroll.

filijonka’s picture

Status: Needs review » Reviewed & tested by the community

per #105

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 107: 2246665-language_interface-107.patch, failed testing.

martin107’s picture

Status: Needs work » Needs review
FileSize
292.82 KB

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);
}
martin107’s picture

Fixed my nit-pick from #110

filijonka’s picture

Status: Needs review » Reviewed & tested by the community

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

filijonka’s picture

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll
martin107’s picture

Status: Needs work » Needs review
FileSize
293.34 KB

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

Status: Needs review » Needs work

The last submitted patch, 115: 2246665-language_interface-115.patch, failed testing.

martin107’s picture

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

martin107’s picture

Status: Needs work » Needs review
FileSize
293.33 KB

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

filijonka’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

Status: Reviewed & tested by the community » Fixed

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

Committed and pushed to 8.x. Thanks!

  • Commit 49a9004 on 8.x by webchick:
    Issue #2246665 by jaredsmith, martin107, YesCT, filijonka, tstoeckler:...
YesCT’s picture

wow! thanks for bringing this home!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.