Updated: Comment #N
Problem/Motivation
In #1497230: Use Dependency Injection to handle object definitions we introduced a basic Language class to keep backwards compatibility with stdClass. In order to keep that simple we did not rename the methods and the properties. Noticed while reviewing #1512424: Add a LanguageInterface, and property setters/getters to Language class.
Proposed resolution
Clean up the language class.
The notes, hints for patchers and hints for reviewers in #2016679: Expand Entity Type interfaces to provide methods, protect the properties might provide more insight here. This is not strickly a child of that meta, but it is doing something similar, so we can consult that meta issue summary and even some of the child issues for understanding.
Remaining tasks
- Change method names and class properties of the Language class to use lowerCamel naming.
- Do a search and replace for updating all Language::WHATEVER_CONST's to LanguageInterface::WHATEVER_CONST's.
- Create and post a patch (https://drupal.org/node/707484).
User interface changes
None.
API changes
?
Not a beta deadline.
Follow-up from #1512424: Add a LanguageInterface, and property setters/getters to Language class.
Comment | File | Size | Author |
---|---|---|---|
#308 | 2226533.308.patch | 4.77 KB | tstoeckler |
#286 | 2226533.286.patch | 276.46 KB | YesCT |
#282 | interdiff.2226533.281.283b-setId.txt | 1.39 KB | YesCT |
#282 | interdiff.2226533.281.283a-directidfrom2188675.txt | 2.29 KB | YesCT |
#282 | 2226533.282.patch | 276.44 KB | YesCT |
Comments
Comment #1
csg CreditAttribution: csg commentedComment #2
csg CreditAttribution: csg commentedPostponed until #1512424: Add a LanguageInterface, and property setters/getters to Language class gets in.
Comment #3
XanoLet's see what this breaks.
Comment #5
visabhishek CreditAttribution: visabhishek commentedLets Check this one
Comment #7
Xano@visabhishek: that is not the goal behind this issue. All methods are to become public. If calling code fails because of this, that means it will need to be refactored to use the LanguageInterface getter and setter methods instead.
Also, whenever you post a patch that is not the first in an issue and is not a re-roll, please provide an interdiff.
Comment #8
XanoComment #10
Xano@visabhishek: The patch from #8 is roughly how to solve this. It replaces many, but not all calls to the former public properties on
\Drupal\Core\Language\Language
with calls to the methods on\Drupal\Core\Language\LanguageInterface
. Apart from converting any remaining property access, we will also need to convert type hints to\Drupal\Core\Language\Language
to\Drupal\Core\Language\LanguageInterface
Comment #11
XanoComment #13
visabhishek CreditAttribution: visabhishek commented@Xano: Thanks for the IDEA, i will try to resolve this
Comment #14
XanoThis patch should at least make sure Drupal can be installed, which makes debugging easier.
Comment #16
XanoComment #18
YesCT CreditAttribution: YesCT commented@visabhishek the notes, hints for patchers and hints for reviewers in #2016679: Expand Entity Type interfaces to provide methods, protect the properties might provide more insight here. This is not strickly a child of that meta, but it is doing something similar, so we can consult that meta issue summary and even some of the child issues for understanding.
Comment #19
Xano16: drupal_2226533_16.patch queued for re-testing.
Comment #21
XanoComment #22
tstoecklerHm.. I wasn't aware of this issue :-/ It seems it in part duplicates #2246665: Typehint with Drupal\Core\Language\LanguageInterface instead Drupal\Core\Language\Language (or I suppose the other way around, but whatever...)
Comment #23
XanoThis issue is about completely replacing references to the class with references to the interface, where appropriate.
PHPUnit tests pass locally. Let's see what Simpletest does.
Comment #25
XanoComment #27
XanoComment #29
Xano\Drupal\Core\Language\Language::sort() is a tricky piece of code that only works on arrays of languages that have the same properties as the default class.
Comment #30
XanoComment #32
XanoComment #34
XanoComment #36
filijonka CreditAttribution: filijonka commentedreroll of #34. couldn't get an interdiff.
Comment #38
filijonka CreditAttribution: filijonka commentedah there was a old function name left. will fix that tomorrow.
Comment #39
XanoMix-up with another patch? Here's a proper re-roll of #34.
Comment #40
XanoComment #43
XanoComment #45
filijonka CreditAttribution: filijonka commentedJust needed to reroll, waiting for the test to finish which I expect to fail actually...and after that begin to sort the tests that are failing
Comment #47
filijonka CreditAttribution: filijonka commentedStill some stuff that needs to be changed. There seems to be a lack of get/set for the locked property which is needed.
Comment #49
filijonka CreditAttribution: filijonka commentedanother patch that won't go through but hopefully there is only one issue left that has to do with the LanguageInterface and that is the lack of get/set functions for property locked.
All other failings are not due to this change...
Comment #51
filijonka CreditAttribution: filijonka commented49: 2226533_49.patch queued for re-testing.
oh sorry apparently missed head by some inches so rerolling this patch
Comment #53
filijonka CreditAttribution: filijonka commentedreroll of 47 and applying the changes I made for patch 49
Comment #55
filijonka CreditAttribution: filijonka commentedComment #56
filijonka CreditAttribution: filijonka commentedreroll of #53, couldn't get an interdiff at the moment. Think something is seriously wrong with this since a lot of errors is coming from tests that has nothing to do with languageinterface to do
Comment #57
filijonka CreditAttribution: filijonka commentedComment #59
filijonka CreditAttribution: filijonka commentedok well this is kinda irritating and impossible when whatever new patch committed to core invalidate this patch.
is there a way to strip down this patch into smaller once?
Comment #60
rodrigoaguileraI think you should assign this issue to yourself aslong as you are working on it
Comment #61
martin107 CreditAttribution: martin107 commentedReroll.
Comment #62
martin107 CreditAttribution: martin107 commentedLooking to override #61 with fixes
Comment #63
YesCT CreditAttribution: YesCT commented@martin107 if you can get into irc and join #drupal-contribute that would be awesome.
Comment #66
YesCT CreditAttribution: YesCT commented#61 failed with
and the interdiff for 61 and 62 is in user.install:
seems reasonable. :)
----
now #62
has
Drupal\comment\Tests\CommentTranslationUITest 127 passes 52 fails 11 exceptions
Drupal\config_translation\Tests\ConfigTranslationUiThemeTest 15 passes 3 fails 1 exceptions
Drupal\config_translation\Tests\ConfigTranslationViewListUiT 30 passes 3 fails 1 exceptions
Drupal\config_translation\Tests\ConfigTranslationListUiTest 114 passes 32 fails 6 exceptions
Drupal\config_translation\Tests\ConfigTranslationOverviewTes 55 passes 12 fails 3 exceptions
Drupal\content_translation\Tests\ContentTestTranslationUITes 95 passes 55 fails 12 exceptions
Drupal\content_translation\Tests\ContentTranslationWorkflows 147 passes 12 fails 4 exceptions
Drupal\content_translation\Tests\ContentTranslationSettingsT 112 passes 2 fails 1 exceptions
Drupal\custom_block\Tests\CustomBlockTranslationUITest 105 passes 50 fails 11 exceptions
and then...
Drupal\system\Tests\Entity\EntityUUIDTest 88 passes
Fatal error: Call to a member function getString() on a non-object in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/config_translation/lib/Drupal/config_translation/Tests/ConfigTranslationUiTest.php on line 597
FATAL Drupal\config_translation\Tests\ConfigTranslationUiTest: test runner returned a non-zero error code (255).
and
Drupal\file\Tests\RemoteFileSaveUploadTest 254 passes
Fatal error: Cannot access protected property Drupal\Core\Language\Language::$locked in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/system/lib/Drupal/system/Tests/Form/LanguageSelectElementTest.php on line 64
FATAL Drupal\system\Tests\Form\LanguageSelectElementTest: test runner returned a non-zero error code (255).
Drupal\system\Tests\Form\FormObjectTest 43 passes
...
and more...
Comment #67
YesCT CreditAttribution: YesCT commentedwe should probably use id(), not getId(), until #2246695: replace all core usages of id() with getid() is resolved.
Comment #68
filijonka CreditAttribution: filijonka commented@yesct if you keep on looking for the fatal erros in #62 which seems to pretty the same as I have there are some of them questionable if they really are language based.
about the id and getid. in the issue you referenced to if you read my comment again you'll see that I point out that in quite a lot of issues for creating get/set functions people have used id() + getid() and some cases only getid().
Comment #69
martin107 CreditAttribution: martin107 commentedI've had a careful look at the difference between #56 and #62 nothing jumps out to explain my failed rebase ....
Looking at CommentTranslationUITest there are two exceptions :-
and
both cases relating to bad offsets "it" and "fr" in an piece of code like :-
$entity->translation[$langcode]['uid']
$langcode is the bad offset
This was saved by ( that is a language_save() ) in ContentTranslationTestBase::setupLanguages()
So something in the save/read is not longer working .. hmm
It late on Friday night for me ... it might be slow to unpick my mistakes...and this might be just one of many errors.
I will look at this again on Sunday ... sorry I can't look at it on Saturday
Comment #70
YesCT CreditAttribution: YesCT commentedoh, I thought this was adding getId().
It's not! :) Sorry. So using getId() seems fine in this issue.
Comment #71
martin107 CreditAttribution: martin107 commentedThe reroll lasted less than 24 hours! This is a reroll of #62
I think I have made better choices this time...in the area of ContentTranslaionManagerAccessCheck::access() .. Here is what it would look like after patch
Please shout if anything seems obviously wrong.
Let's see what testbot thinks...
Also from review in #66 - Fatal error: Cannot access protected property Drupal\Core\Language\Language::$locked ..
That look like an isolated problem which could be cherry picked but I am leaving this as a straight reroll just to avoid confusion
Comment #73
filijonka CreditAttribution: filijonka commentedhi thanks for working on this @martin107, 24h is actually quite long time in this issue.
ehm was going to point out the small problems but I did a new patch continueing the work I had done simply cause I wanted to know what was going on. added that patch to a test issue to see what happends and it turned out to have the similar problems that patch in 71 have.
After a chatt with @yesct she suggested to diff my patch and #71 + ul the patch.
This patch will fail as #71 on same fatal errors, getvalue and getstring. and as earlier a lot of language tests are failing.
Comment #75
martin107 CreditAttribution: martin107 commentedNo major fix - Correcting access to protected properties. Reduces the exception count a bit.
Comment #77
martin107 CreditAttribution: martin107 commentedMore issues fixed in the same vein as #75
except in answer to reported error "Undefined LanguageInterface"
Others might think this is really saying the "use" statement was missing from the top of the file!
Comment #78
martin107 CreditAttribution: martin107 commentedComment #79
YesCT CreditAttribution: YesCT commentedthis is taking out the only usage (besides the test) of sortByWeightAndTitleKey()
why?
patch #30 was the one to introduce the change
in core/lib/Drupal/Core/Language/Language.php
adding some sort related issues.
also, if the typehint issue gets in first, this should be easier to review. adding it as related.
Comment #81
YesCT CreditAttribution: YesCT commentedI think these should both be isLocked()
Comment #82
filijonka CreditAttribution: filijonka commentedbeen discussing this with @yesct and I think that the best approach to the Language::sort is to let it be as it is on head instead of the changes this patch is doing. simply because there are issues like #2239399: Languages should be sorted by label instead of title dealing with this.
The tests are written for that solution and if we change it and doesn't change the tests they are going to fail.
Comment #83
martin107 CreditAttribution: martin107 commentedBreaking this interdiff into stages for review
1) As testing get further .. exposed more issues with accessing protected properties..interdiff-75-getters.txt
2) Fixed #81 ( hanging my head -- sorry for the distraction ) ...interdiff-75.locked.txt
3) Credit to filijonka for this code tidy ... interdiff-75-tidy.txt
Comment #85
martin107 CreditAttribution: martin107 commented1) Fixed more access to protected methods ... via getters.
2) No more white space.
I will sit on this .. until it goes green
Comment #87
martin107 CreditAttribution: martin107 commentedCorrected a few more $language->getName() and $language->isLocked()
Ahem should have used search/replace for this a bit sooner.
Comment #89
filijonka CreditAttribution: filijonka commentedtried the reverting this back to using the language::sort that is on head and that created a lot of errors.
basicly all test just gave that sort(null) instead of sort(array of languages).
So let's ignore that for now and see where we end up when it's green. perhaps these issues dealing with this function is sorted out by then.
this is yet another patch for the changing that we have begun.
Comment #90
filijonka CreditAttribution: filijonka commentedooh needs review...silly moi
Comment #92
filijonka CreditAttribution: filijonka commentedThe error above was caused by a check in language.module saying if(!empty($language->locked)) which I changed to correct function but since locked is a boolesk variable it should be sufficient to just do if(!$language->isLocked())
fixed that and did some more changes..
Comment #94
filijonka CreditAttribution: filijonka commentedhmm had added a whitespace by accident and hopefully that was the reason for prev. failure.
I noticed somewhere that we had a line saying $language->direction ? ... : ...; and that was changed to getDirection.
but direction isn't a boolesk variable and afaik never has.
So my guess is that we're getting close to a good result but there are probalby some logical errors involved too.
Comment #96
filijonka CreditAttribution: filijonka commentedyes this is a logcial error that turns up. working on it..
Comment #97
martin107 CreditAttribution: martin107 commentedI have a fix for one of the test errors....from #87 ( Drupal\node\Tests\NodeTokenReplaceTest )
this->languageInterface->id becomes this->languageInterface->getId() in two places see below..
I will leave it here so we can come back to it.
But it highlights some non-core standard code...
$this->languageInterface->getId() !!!! ($languageInterface needs a name change )
There has been some discussion about how to reduce the size of the patch ... I will file a minor issue just for the name change.
rather than making our lives harder ( PS #2270339: $languageInterface is a misleading variable name in TokenReplaceUnitTestBase )
Comment #98
filijonka CreditAttribution: filijonka commentedreverted some of the previous changes made by me and did some more changes.
Comment #100
filijonka CreditAttribution: filijonka commentedsome of the current errors seems to be due to the tricky part that we have two different kind of language interfaces. one in module and one in core.
Comment #101
filijonka CreditAttribution: filijonka commentedComment #103
martin107 CreditAttribution: martin107 commented1) Just wanted to comment for the record that #98 slips in the fixes from #97 thank you.( I was just about to do that )
NodeTokenReplaceTest is now green..
2) Its been suggest on IRC that I keep this issue updated with the concepts being developed as common currency in #2270339: $languageInterface is a misleading variable name in TokenReplaceUnitTestBase
$languageInterface is being replaced by $interfaceLanguage
As a way differentiate between the two
A) $interfaceLanguage the interface settings being used for the test and created :-
$this->languageInterface = \Drupal::languageManager()->getCurrentLanguage();
B) LanguageInterface:( @see \Drupal\Core\Language\LanguageInterface AND \Drupal\language\LanguageInterface )
As always naming things is hard, and the more eyes on this the better... so please direct comments to the other issue. I am aiming to put the other issue to bed soon after the 22nd of May
Comment #104
YesCT CreditAttribution: YesCT commented1.
#101 looks right, cause that language is a LanguageEntity which is a language/LanguageInterface (configurable)
which #2246679: Make Language module's LanguageInterface (to be ConfigurableLanguageInterface) extend Core's LanguageInterface will help clarify.
but we should probably revert it all the way back to what it was.
doing that.
2.
also,
fixing the typo
gtetId
from #89
Comment #106
YesCT CreditAttribution: YesCT commentedWould have thought that would have fixed more than one fail.
Maybe the gtegId is in a section of code that isn't used, or isn't tested?
Comment #107
filijonka CreditAttribution: filijonka commented@yesct the reason for at least 140 of these fail and probably more is that in the language.module there are using both of the languageinterfaces. So sometimes it uses Drupal\Core\Language and sometimes it uses Drupal\Language\Language(?) Not sure I got the last one correct but basicly it means that sometimes it is e.g ->getId() and othertimes its ->id.
That is what I'm doing at the moment to figure out these errors.
Comment #108
martin107 CreditAttribution: martin107 commentedPatch no longer applies ( this is just a straight reroll - lots of merging, auto merging and 2 trivial conflicts to resolve )
So that patch lasted about 4 hours -- Who knew Europe would wake up and go on a commit spree...
Comment #110
filijonka CreditAttribution: filijonka commentedjust reverted back some of the changes made in language module. went it through and think this would make it much better. i hope
Comment #111
martin107 CreditAttribution: martin107 commentedSo I hope to shine a spotlight on the ->getId() versus ->id debate - As it too is kicking me in the bum.
Looking at the recent conversion of book_language_entity_delete ( see book.module )
the function argument when I followed it back to its source function .. takes me to \Drupal\language\Entity\Language
where the magic getter function facilitates access to the id :-
So that should be reverted along with a few others I can see...
This is opposed to code of the form
where the getId() can be traced to \Drupal\Core\Language\LanguageInterface. An separate Interface.
PS sorry cross post filijonka has beaten me to the fix
Comment #113
filijonka CreditAttribution: filijonka commentedComment #114
filijonka CreditAttribution: filijonka commentedas soone as this test is done there will be a new patch which solves a big amount of errors I think.
Comment #116
filijonka CreditAttribution: filijonka commentedComment #118
filijonka CreditAttribution: filijonka commentedmade also sure it's still on head
Comment #120
filijonka CreditAttribution: filijonka commentedreroll
Comment #122
martin107 CreditAttribution: martin107 commentedAs advised by filijonka this reroll is based on #118
Lots of merging, auto-merging and conflicts in rebase ...
Mostly this is a happy reroll because the side issue raised in #103 has landed in core.
The $languageInterface is being replaced by $interfaceLanguage issue.
5 conflicts in this reroll led to the selection of 4 tests for local testing as always testbot will give a more complete answer ...
So the target is is back to the 48 fails of #118...
PS
I have been asked by the community to ensure that the distinction between ->id and ->getId is preserved, and is a concern myself...
The answer is it an ongoing thing that will come out right in the wash....
There are drives in separate issues show clear differences between
\Drupal\Core\Language\LanguageInterface and \Drupal\language\Entity\Language
When these initiatives are complete and have caused more 'Happy Rerolls' here then I will take the time to visual inspect this monster of a patch and post further comments
PPS see the fail below.... test bot reports out of memory errors will force retest again soon....
Comment #124
filijonka CreditAttribution: filijonka commented122: 2226533_122.patch queued for re-testing.
Comment #126
martin107 CreditAttribution: martin107 commentedI am at the limit of what I can do tonight... so I am just pushing the obvious fix.
I am "unassigning" but not changing the amount I work on this .. I am still committed to lurking on this until its is green and stable.
BUT because YesCT and filijonka are working on this just as hard and I don't want to permanently signal that I am always working at this moment
I will try some more in about 12 hours.
Comment #127
martin107 CreditAttribution: martin107 commentedComment #129
martin107 CreditAttribution: martin107 commentedDrupal\locale\Tests\LocaleContentTest now passes.
As a side note I have changed a local variable $language_interface to become $interface_language ( it holds a language object not an interface )
I know it should be $interfaceLangauge but then it would be out of keeping with the other un-CamelCased local variables in the file!
Comment #130
martin107 CreditAttribution: martin107 commentedtriggering testbot
Comment #132
filijonka CreditAttribution: filijonka commentedok so now back to 40 fails, different between this and #87 are that errors like fatal errors on getString and getValue are gone. Which was what I hoped for but also hoped that we would be a little be less than 40.
Comment #133
martin107 CreditAttribution: martin107 commentedCherry picking something obvious resulted in
./core/modules/system/lib/Drupal/system/Tests/Entity/EntityTranslationTest.php going green
Interface constants should be ONLY pulled from the interface directly not through an intermediate object
So changes are an iterations of the form
PS I used git diff -M to create the patch .. but I am a bit suspicious about the file size ( 50kb versus 500kb )
Comment #134
filijonka CreditAttribution: filijonka commentednice catch! haven't seen those! great work @martin107
this is strange but doesn't really matter but those changes that are made in latest patch is already done in #118. I was looking at that local branch wondering why my grep didn't return these and they were already changed to LanguageInterface..
oh well still a good catch
Comment #136
filijonka CreditAttribution: filijonka commented@martin107 You need to do a diff without the M flag and mostly likely you need to reroll it to.
Comment #137
filijonka CreditAttribution: filijonka commented129: 2226533_129.patch queued for re-testing.
Comment #138
filijonka CreditAttribution: filijonka commentedok asking for a retest on #129 simply because when I applied that patch the changes that are in #133 interdiff are already there + when doing the tests locally they don't give the same results as in #129. So wanna see the result one more time
Comment #140
martin107 CreditAttribution: martin107 commented@filijonka so I understand from IRC that some of the tests pass locally for you
Could you post your PHP version ...
I am storing a link to the relevant test results here as the current links maybe overwritten by future retesting
https://qa.drupal.org/pifr/test/792483
testbot was php5.4
Comment #141
filijonka CreditAttribution: filijonka commentedlet's see what happends on testbot. I'm running on php 5.4.x as bot is.
the test parameter I changed is simply because locally the verbose message showed that there were more than 1 user on the system and since I used both cleaning the caches with drush and removed temporary tests we now it can exist more than 1 user. I'm just making sure to take a high number here and locally it got green then.
edit: adding that I don't think that is the solution I would just like to confirm if that is the case.
Since I don't know how this works on drupal but my thought is that if we really want to make sure that this test should pass we should choose a high number. In the back of my head it's simply like this: Can we now that all previous tests has cleaned up after itself so the setup for this test is the one adding a user?
Comment #143
filijonka CreditAttribution: filijonka commentedhopefully this is down to one error.....
Comment #145
filijonka CreditAttribution: filijonka commentedof course another reroll, auto-merged everything so no conflicts to handle
oh forgott to pull..coming new reroll soon
Comment #147
filijonka CreditAttribution: filijonka commentedok something must be very wrong with patch 145 so redid this based on patch 143
I've not seen this line @147 before in language.module but can't really find when it was introduced but I don't think it wasn't recently.
is_new is not a property of language as far as i can see but we should get fatals in that case..well let's see what the bot says
Comment #149
jiv_e CreditAttribution: jiv_e commentedComment #150
jiv_e CreditAttribution: jiv_e commentedSorry! I didn't have time to take this further in the sprint. An other issue came back to haunt me.
Comment #151
martin107 CreditAttribution: martin107 commentedPlease ignore. #147 is the active patch.
Comment #153
martin107 CreditAttribution: martin107 commentedStraight reroll of #147.
Comment #155
martin107 CreditAttribution: martin107 commentedReroll form #147 because my recent patches had more errors..
This is a PSR-4 reroll!
Comment #157
YesCT CreditAttribution: YesCT commentedDid you do it using the steps at:
https://groups.drupal.org/node/424758
?
Comment #158
martin107 CreditAttribution: martin107 commented@YesCT yes I did... although it looks like I messed something up .. locally I had test passing related to all conflicts I had to resolve!!!
Ii looks like HEAD moved on between submission and testbot testing.
Given :-
1) This issue has a history of frequent re rolling .. I notice several comments appealing for strategies on how to break this patch into parts..
I still have no useful insight into this question :-(
2) The commit spree juggernaut that is Austin is about to start.
I think this issue should be put aside for at least a week.
Comment #159
martin107 CreditAttribution: martin107 commentedI starting to reroll this now
Comment #160
martin107 CreditAttribution: martin107 commentedNow that #2246665: Typehint with Drupal\Core\Language\LanguageInterface instead Drupal\Core\Language\Language has been committed, lots of the changes that were made in both patches need to be unwound.
This patch is 40% smaller and should cause less conflicts in the future and so be easier to reroll.
I rerolled from #153 the last patch to at one time pass all tests.
This is not a happy reroll... When rebasing there were 120 conflicts to resolve! ( individual changes not files )
I am certain there will be some errors ... where there were non-trivial decisions to be made I tested the function until it passed
I am not expecting this to pass first time..
ALSO
Note in many files CoreLanguageInterface in this patch has become BaseLanguageInterface to reflect the current state of HEAD
Comment #162
martin107 CreditAttribution: martin107 commentedsome knots and kinks have been brushed out...
1) I fixed up these files in light of the PSR-4 changes.
core/modules/field/lib/Drupal/field/Tests/FieldInfoTest.php -- removed
core/modules/node/lib/Drupal/node/Controller/NodeController.php -- change ported to correct file and duplicate deleted.
core/modules/node/lib/Drupal/node/NodeStorage.php -- both changes already in core, duplicate deleted.
I made a patch at this point to see how much more of a reduction this would make ( the patch was 55% of the #153 )
2) I backout some ... ahem reroll confusion from the following files.
TranslationLinkTest.php
LanguageServiceProvider.php
3) Duplicate use statement reported in test failure was removed.(#160)
Running all the unit test did not expose any problems BUT my expectation is there are more problems lurking...
Comment #164
martin107 CreditAttribution: martin107 commentedMy apologies For the record here is the interdiff I omitted.
Comment #165
martin107 CreditAttribution: martin107 commentedTesting of the last patch ran only a little further and exposed another duplicate use statement issue similar to #162.3
I have listed all files changes in this patch 'git diff 8.x --name-only'
and through a little sed hackery I have run php -l on all relevant files...
So I don't think there are anymore linting issues like this for the test bot to find.
Does anyone know of a more convenient way of running the syntax checking part of testbot on my local machine ?
I was kind of expecting that running unit tests would identify these problems for me but hey-ho
So this time testbot should run deeper and expose more complex flaws..
Comment #167
martin107 CreditAttribution: martin107 commentedSmall advance EntitySerializationTest.php now passes.
Comment #169
martin107 CreditAttribution: martin107 commentedI won't be able to look at this for a few days ... I see echoes of past failures ... so does filijonka ..
when i return to this my next step is to look at a diff with #143 and look for correlations.
Comment #170
martin107 CreditAttribution: martin107 commentedThis is just a straight reroll
Clearied 3 conflicts and in part, the patch became slightly simpler now that
#2281905: Stop caching plugin discovery/info hooks by language has been committed
So my expectation is a return to 11 fail(s), and 0 exception(s).
Comment #172
martin107 CreditAttribution: martin107 commentedsorry cross post
Comment #173
martin107 CreditAttribution: martin107 commentedReroll.
Core swapped check_plain() for String::checkPlain() in many places.
Comment #175
martin107 CreditAttribution: martin107 commentedBig trouble, little fix.
Comment #177
martin107 CreditAttribution: martin107 commented6 hours and 2 commits later, this needs a reroll
The real culprit :-
#2256679: Use config schema to determine which config entity properties to export regardless of whether they are public or protected
Comment #179
martin107 CreditAttribution: martin107 commentedcleared syntax error.
Comment #181
martin107 CreditAttribution: martin107 commentedReroll.
Comment #183
martin107 CreditAttribution: martin107 commentedjust keeping this patch upto date :-
CommentTypeTest and EntityFieldDefaultValueTest now back to green
Comment #185
martin107 CreditAttribution: martin107 commentedReroll.
Comment #187
martin107 CreditAttribution: martin107 commentedhmm not sure these changes can be justified.
Comment #188
martin107 CreditAttribution: martin107 commentedThe patch no longer applied .. so reroll ... made following change to keep things upto date
Next
I have found one bug in content_translation.module
./core/modules/content_translation/src/Tests/Views/TranslationLinkTest.php now passes.
PS regarding my comments in #187 yes they can all be justified...
Comment #190
martin107 CreditAttribution: martin107 commentedregarding the lint failure from #188
for a full explanation see
http://stackoverflow.com/questions/1075534/cant-use-method-return-value-...
I was using php5.5.10 locally and so that is why everything looks ok for me.
I will post a patch in the morning
Comment #191
martin107 CreditAttribution: martin107 commentedquoting from http://kunststube.net/isset/
so replacing
with
is directly equivalent .. these things can be tricky
Comment #192
martin107 CreditAttribution: martin107 commentedReuploading patch, in an effort to get testbot to act.
Comment #195
martin107 CreditAttribution: martin107 commented#185-#192 needs a summary.
Despite starting with 6 failures and ending with 10 that was actually a minor positive step forward..
Drupal\system\Tests\Entity\EntityQueryTest is an intermittently failing test which accounts for 5 extra fails when it appears.
In short one repeatedly appearing bug has been fixed...
Note the differences between php5.5 and php5.4 handling of empty do not explain EntityQueryTest's intermittent nature. In theory or in practice.
I have looked back through the logs and can see those tests both passing and failing on when testbot was on php5.4.
But indirectly the empty() issue led me to search and find the next bug.
For me locally this next fix changes EntityQueryTest tests from pass to fail.
Comment #197
martin107 CreditAttribution: martin107 commentedI am reverting a buggy change from the patch...
From locale.module here is the interdiff -
A) The wrong language is modified.
$language relates to \Drupal\language\Entity\Language and not Drupal\Core\Language\Language which is the subject of this patch.
B) the condition of the if statement is altered
!empty($language->locked ) is the INVERSE of
!$language->locked
This affects many tests and removing this flaw exposed other issues... it may not decrease the error count but I am using testbot to uncover all the bugs lurking behind this one!
Comment #199
martin107 CreditAttribution: martin107 commentedBingo!!! Drupal\config\Tests\ConfigLanguageOverrideTest is now passing locally.
Many of the failing tests used language_save(). I have removed an out of scope change/bug from the patch which was causing a logical flaw.
language_save() :-
1) Before saving the $language_entiy save the state from isNew()
2) Save updates to the $language_entity
3) Working version: was using state saved in step 1 to determine if overrides should be installed.. buggy patch calling isNew() AGAIN and getting the wrong result...
Comment #201
martin107 CreditAttribution: martin107 commentedReroll caused by #2291777: Random fail in LanguageFallbackTest being committed.
In rewriting a test alexpott removed the need for some of this patch's changes...
Comment #203
martin107 CreditAttribution: martin107 commentedI can install current head, BUT when I apply the patch and then drush si ... it fails ( or install time blows up )
I am just triggering testbot to see if it can see any problems.
Current HEAD :-
Commit: 234ed4f894c3cd27f6bc7e2e26b58b3cc38d61be [234ed4f]
Issue #2226267 by yched: Improve default value handling of fields to be consistent (follow-up).
Date: 25 June 2014 13:38:29 BST
Comment #204
martin107 CreditAttribution: martin107 commented201: 2226533_201.patch queued for re-testing.
Comment #206
YesCT CreditAttribution: YesCT commentednot really novice given the size and some of the tricky confusions.
also, doesn't apply. I'm rerolling now.
Comment #207
YesCT CreditAttribution: YesCT commentedjust a reroll.
nothing tricky.
can ignore the conflict diff. playing with what might actually make sense.
conflicted with
#2272467: Remove usages of watchdog() from procedural code
#2286681: Make public properties on ConfigEntityBase protected
Comment #209
YesCT CreditAttribution: YesCT commentedlooks like only the random failures left
#2254011: Drupal\system\Tests\Entity\EntityQueryTest random failure
Comment #210
martin107 CreditAttribution: martin107 commentedThis need a reroll.. I will try and find time tomorrow to both reroll and implement this change.....
But tonight I have only time for a small review...
in more than a few posts
https://www.drupal.org/node/2254011#comment-8930337
https://www.drupal.org/node/2226533#comment-8903649
etc
The use of !empty($language->locked ) appears in several places dotted throughout the patch.
The empty() wrapper should be moved into the isLocked() function :-
A) This is a maintenance hazard as someone will have to identify and uniformly change all instances, should the policy change to say using isset() instead.
B) the subtle change in definition of empty() between php5.4 and php5.5 will be avoided.
Comment #211
martin107 CreditAttribution: martin107 commentedI have not been able to spend as much time on this as I would like recently but here is a reroll.
no conflicts just auto-merging etc
Comment #213
martin107 CreditAttribution: martin107 commentedSo there is some circumstantial indications that empty(Locked) maybe a factor in the long running bug hunt that is
https://www.drupal.org/node/2254011#comment-8930337 but those largely relate to \Drupal\language\Entity\Language objects
not Drupal\Core\Language\Language objects
That was only important here because in the past I/we have mistakenly altered the empty($language->locked) statements before backing out the changes.
I hope that is clear :-)
Now I am talking about a minor code polish to Drupal\Core\Language\Language objects and the way isLocked() is handled.
Looking through core there are plenty of status(), hasTranslation() isNew() type function that use !empty() to implicitly cast the return to a bool.
Understandably it is employed where there is a complex coming together of several factor where keys may or may not be present( in an array for example. )
BUT
locked is an uncomplicated CONSTANT defined during construct().
There are only 2, other places that I can see that use this, slightly paranoid practise, to protect a boolean with such a simple lifecycle.
[ ConfigEntityBase.php status() and isNew() ]
So do we really need the empty()?
Then I looked at our own house and see that technically yes even we screw it up!!!
isLocked is initialized to be a FALSE by
So FALSE will be the default when no value is supplied to the construct()
but then upon construction we override with and integer!!!!
We could go for a if statement in the constructor to cast the value, whatever it is, to a bool, and this would protect against inputs like NULL
but that approach seems too complicated.
This solution is simple the interdiff small, normally I would accept that this change is clearly "out of scope", but I am looking ways to counter the charge that
This issue is the only one that routinely exposes the problem, and so our errors maybe, simply the result of our own mistakes, and not necessarily the subtle exposure of a recurring design instability.
So I want to contribute to settling that question, one way or the other, in part by demonstrating that we have taken sufficient care to sweep away all irrelevant complications, or clouding factors.
Comment #214
martin107 CreditAttribution: martin107 commentedtriggering testbot
Comment #216
YesCT CreditAttribution: YesCT commentedwhat if we take out all ->locked changes and see if it passes?
If it does pass, we can open a separate issue to deal with ->locked, and move on from here.
Comment #217
martin107 CreditAttribution: martin107 commentedThanks that is clear way to separate out issues, in a way I had not thought of ... it will be a few days before I can get to it.
Comment #218
YesCT CreditAttribution: YesCT commentednoticed this small typo.
Comment #219
YesCT CreditAttribution: YesCT commentedan experiment to see if taking out the locked changes gets this to pass. I double checked (and triple checked) to make sure I got them all... but might not have.
Comment #221
martin107 CreditAttribution: martin107 commentedAll credit goes to YesCT for so elegantly piercing my assertion..about isLocked
YesCT++
Oh oh regarding my misspelling of isLlocked my inner troll cannot help but suggest that I made the same spelling mistake systematically
when creating the wikipedia page for the seaside town of Llandudno ( http://en.wikipedia.org/wiki/Llandudno )
Comment #222
martin107 CreditAttribution: martin107 commentedRemoved changes to core/modules/block/src/Tests/coreDevs
looks like I let this slip in between #199-#201
Comment #223
YesCT CreditAttribution: YesCT commentedwhat? it's green?
I'm playing in #2274167: [ignore] Patch testing issue with this.
this is 213 (with the isLocked), with the accidental devs file removed as in 222, but with two typo fixes. interdiff is just the typo fixes.
Comment #224
YesCT CreditAttribution: YesCT commentedwhy these changes? I think those are config language and not the core language class.
Comment #226
YesCT CreditAttribution: YesCT commentedreroll since #2293825: Various test classes do not have a "Test" suffix, was an auto 3-way merge. no conflicts.
another patch coming.
Comment #228
YesCT CreditAttribution: YesCT commented1. #143 added a mode change to core/scripts/run-tests.sh
reversing that.
2.
This $language is a language module configurable language (Entity/Language) not a core language. (it's also in a deprecated function)
reversing that.
3. also in language.module
just fixing this nit white space after the if.
4. in language.module in function language_language_entity_insert(LanguageEntity $language) {
this is a change to $language, where it is a configurable language (Entity/Language) and not the core Language class.
reversed this change.
4.b.
two changes similar in
function language_language_entity_delete(LanguageEntity $language) {
Comment #230
martin107 CreditAttribution: martin107 commentedHmm
Having paid close attention I see that the recent changes are good, but I am still looking for a way forward on this given how unrepeatable the patches in this issue are proving see #223 and #2274167: [ignore] Patch testing issue
I have a few tests in mind, before I take the labor intensive step of separating this patch in parts, but see that it needs a reroll first.
Looking at the file diff between last patch and this all changes are of the type :-
< - '#title' => $this->t($definition['label']) . ' (' . $language->name . ')',
---
> - '#title' => $this->t($definition->getLabel()) . ' (' . $language->name . ')',
that is this patch, like head is now using $definition->getLabel()
Comment #232
martin107 CreditAttribution: martin107 commentedOk, so I have found something, I don't have all the answers, just a good story to tell and a new approach which turns the problem on and off
in a repeatable way so that it can be studied. Something we have been sorely missing.
So peviously I have summaried of https://www.drupal.org/node/2254011#comment-8813981 as
We have a complex revisionable-cacheable-database-storage mechanism ....
WHERE in a "write-revise-read" test is the data being corrupted?
To feel my way forward I tried to construct sentences that are true :-
A) Part of the problem maybe the presence of magic getters and setters and how we may be exciting a new found bad path through the caching system.
B) Part of the problem maybe the we may have missed a conversion from say $language->id to $language->getId() and the code then finds the wrong id or a null value.
My thought was well given B, if we return all protected properties to public, the getters will still work, but now any uncorrected direct access, that this issue wants to remove will also flow through the code as before.
In order, here is a list of all properties that have become protected on Drupal\Core\Language\Language objects.
1) name
2) id
3) direction
4) weight
5) default
6) negotiationMethodId
7) locked.
Here is the command I am using to show the problem - locally the test takes 19 seconds to say pass/fail. For me unlike YesCT's work with testbot, repeating the test over and over always yields the same answer.
php core/scripts/run-tests.sh --php /Applications/MAMP/bin/php/php5.5.10/bin/php --color --url http://dev.drupal.co.uk --verbose --file ./core/modules/system/src/Tests/Entity/EntityQueryTest.php
BINGO
if 1, 2, 3, 4, 5, 6, 7, are made public then the failing test passes - so my theory has legs! One of those pesky properties has not been fully converted!!!!
I am telling this long winded story to spark ideas in others and explain what I think - I have not fully explored this
The next test surprisingly narrowed the field down.
With only 1(name) made public and all other returned to protected the failing test still pass !!!! So it looks like name is the problem child.
I am looking for testbot to confirm that $name is the key.
Comment #234
martin107 CreditAttribution: martin107 commentedI have found one instance where direct access to name was still being attempted.
It looks like the name property is being renamed to carry some kind of translatable heading text a curious concept.
but any further comment on that seems out of scope
from core/includes/update.inc
Looking further up the code I can say in all events $default is assigned to be the return of \Drupal::languageManager()->getDefaultLanguage(); which is documented to be of type \Drupal\Core\Language\LanguageInterface, so thats why its fits.
Comment #235
martin107 CreditAttribution: martin107 commented17 commits later, well this patch needs a reroll.
No conflicts, just merging etc.
NB - this is a still a trial patch as $name is declared public.
Comment #237
martin107 CreditAttribution: martin107 commentedJust placing a stake in the ground to define what repeatability will look like when we have it.
I hope I have been clear that there was always a difference between what I see locally and what testbot reports.
HEAD is constantly changing and locally things are no longer as I reported in #232, using :-
and patch 2226533.235.patch
Out of 10 runs 8 fails, 2 passes
Out of 10 runs clearing the cache in between with "drush cr", 8 fails 2 passes.
Now it looks like my locally testing and testbot agree... that is Mostly broken with the occasional pass.
Comment #238
martin107 CreditAttribution: martin107 commentedReroll.
Comment #239
martin107 CreditAttribution: martin107 commentedComment #240
YesCT CreditAttribution: YesCT commentedreroll for #2284103: Remove the request from the container and #2292821: Use widget for comment subject field
ignore the conflictdiff.
Comment #242
martin107 CreditAttribution: martin107 commentedNot the cleanest of rerolls.
Recently many deprecated functions has been removed, some relating to the language class, so the patch size is reduced a little.
if the functionality has just been moved I may have missing something so I don't guarantee success.
Comment #244
martin107 CreditAttribution: martin107 commentedAll but Drupal\contact\Tests\ContactStorageTest seem to have a simple error in common.
Now I have Drupal\contact\Tests\ContactSitewideTest passing, I have high hope for the rest...
BTW CrudHookTest which failed recently in #240, passed in #242 - so that was accidently fixed :)
Comment #246
martin107 CreditAttribution: martin107 commentedA) The practise of daily rerolling this issue in not productive.
B) Myself and others don't consider stepping through the debugger on this issue a good way forward.
C) This issue converts several properties to protected. They can be split off into separate issues.
I think realistically the only way forward on this issue it to redefine it as a META.
Each separate issue will touch many parts of the system and so will need a "Will cause commit conflicts" tag
Any comments welcome, I will be on IRC for the next 8 hours or so
from 232
Comment #247
martin107 CreditAttribution: martin107 commentedAfter a brief chat in #drupal-i18n dropping the meta idea in favour of a series of child issues extracting what we can.
First up the hopefully uncontentious weight property
Comment #248
martin107 CreditAttribution: martin107 commentedStraight reroll.
Comment #250
YesCT CreditAttribution: YesCT commented@martin107 Thanks! To check the reroll I also rerolled 244.
conflicts from
#2287727: Rename FieldConfig to FieldStorageConfig
#1533250: Many coding standards clean-ups in locale module
#2146035: drupalSettings.path.pathPrefix does not contain the language identifier
#597236: Add entity caching to core
1. resolved conflict in FieldTranslationSqlStorageTest by taking out the use re-ordering, and leaving in the renaming of the other class. This is different than the reroll in 248.
2. a conflict from correcting stand of name languageName to language_name. This is different than the reroll in 248.
3. and one from a double space to a single space from that standards clean up also.
4. this was an interesting conflict in LanguageNegotiationUrl.php
as it seems like what was a return was just setting the first part. so I wonder if we needed to find the rest of the logic from the ternary.
from #2146035: drupalSettings.path.pathPrefix does not contain the language identifier
but
Is the complete change... so I guess there nothing added later to the logic there that we need to adjust.
5. we didn't need the addition of the $entities typehint since 597236 added the @param docs for it.
this makes my reroll different than the one in #248
Comment #251
martin107 CreditAttribution: martin107 commentedNow that #2304651: Core Language sort() should not work, needs to use name instead of default title has been committed this needs a reroll.
Comment #253
martin107 CreditAttribution: martin107 commentedLanguage::sort fixes.
Comment #257
martin107 CreditAttribution: martin107 commentedI currently have no idea on how to proceed with the EntityTranslationTest failures, but in the mean time, the patch needs rerolling.
Trivial conflicts in toolbar.module were causing conflicts.
Comment #259
martin107 CreditAttribution: martin107 commentedSingle fix restores all tests but EntityTranslationTests.
Comment #261
YesCT CreditAttribution: YesCT commented1.
rerolling for
#2313627: FormStateInterface needs @ingroup
#2218313: Toolbar subtree is broken if you use a different language.
#2225353: Convert $form_state to an object and provide methods like setError()
#2149197: Replace format_interval with \Drupal::service('date')->formatInterval()
#2301319: MenuLinkNG part5: Remove dead code; and party! will do an interdiff to look for places in the menu stuff to that might need to use methods.
2.
2a.
Note this conflict is something I want to keep in mind
and relates to #2239399: Languages should be sorted by label instead of title and #2304651: Core Language sort() should not work, needs to use name instead of default title
2b.
This patch was:
2c.
completely removing the only use of sortByWeightAndTitleKey().
and changing it to sort by weight and then then name.
we should not have to do any of this now that #2304651: Core Language sort() should not work, needs to use name instead of default title is in. so this reroll is undoing that since 2304651 got committed.
2d.
the missing return should be added by another issue, maybe something to go through that whole file and look for any @param or @return or typehints that are wrong/missing. (we might be able to also change that typehint for the @param on sort to be LanugageInterface[] instead of just array... anyway, out of scope for this issue.
2e.
check usages of sort() that this patch may have had to change that now also need to undo.
like
in core/modules/language/src/ConfigurableLanguageManager.php
(note here we are again getting language module configurable languages confused with core language languages.
this looks like a bug in core. I checked the type of things that are in $this->languages
It comes from
class ConfigurableLanguageManager extends LanguageManager implements ConfigurableLanguageManagerInterface {
the core LanugageManager
still troubling. but putting back to just
Language::sort($this->languages);
also in core/includes/update.inc
-----
3.
3a.
phpunit on 261 says:
OK, but incomplete, skipped, or risky tests!
Tests: 5345, Assertions: 9527, Incomplete: 1.
3b.
but that is happening in head also, so not the fault of this issue.
3c.
ran with --tap and it said:
not ok 2250 - Drupal\migrate_drupal\Tests\source\d6\NodeRevisionTest::testRetrieval # TODO Incomplete Test
-----
4.
4a.
Looking into what might be different from my and the other recent rerolls.
so I also rerolled 259... :)
and the only diff between a rerolled 259 and my 261 was:
which is probably a good additional change and the result of menu link changes of various parts.
4b.
So did that also to my 261.
4c.
note that the typehint is wrong on
function menu_ui_node_save(EntityInterface $node) {
it actually has to be Node (NodeInterface is not enough)
(ContentEntityBase is what requires that getUntranslated() exists on the thing)
Maybe a separate follow-up issue needed here.
Not changing the type hint here.
4d
So I'm going to go through all the changes from the various parts of the menu link and look for code that have needed language changes.
But ... if we have any tests that execute code that uses a property, it would have caused test failures since the properties are protected in this issue.
(I'm on a plane and am now wishing I would have opened the test results in another tab, so I could see where the fails were.. have to do that when I land.)
in part4 there was this change
which @martin107 caught. great.
I didn't see any others in part 1, 2, 3 or 5 (which only removed code)
-----
5.
5a.
checking any changes in core since 250 that may have added direct usages of properties that are now protected.
in #2216535: Replace Node overview topic and Node API topic with Entity Hooks topic
in core/modules/system/entity.api.php
it added this in comments:
5b.
I'm not sure of the type of that $language
Something similar is in core/modules/system/core.api.php
ah, taggled stuff that will get better when we force configurable languages to also extend the core language interface (and make the properties on configurable languages protected also.
5c.
Ignoring this for now. But maybe it needs a follow-up issue to look into it.
-----
6.
I didn't see any more changes in head.. so really need to look at those fails and submit this 261 to the bot (cause this sort() stuff could be causing it)
-----
7.
Completely unrelated, just making a note here. Will make a separate issue or ask on the formstateinterface one.
Look into this later.
Why was
/**
* {@inheritdoc}
*/
- public function getSourceLangcode(array $form_state) {
+ public function getSourceLangcode($form_state) {
return isset($form_state['content_translation']['source']) ? $form_state['content_translation']['source']->id : FALSE;
}
not hinting with the new FormStateInterface?
=========
in summary, the diff between 259 and 261 is how the sort() is handled.
let's see what the testbot says.
Comment #263
martin107 CreditAttribution: martin107 commentedRegarding #261 7. form_state type hinting is resolved.. soon in #2313823: Use FormStateInterface for all typehints
Comment #264
martin107 CreditAttribution: martin107 commentedUghhh. Sorry it has taken a little while for the penny to drop...
Please forgive the long winded description I hope it will jar someone's brain into providing the answer.
A) Its seems we have traded the EntityQueryTest problems for EntityTranslationTest problems
Ignoring rerolls the only change was arround the function
\Drupal\Core\Language\Language
B) When #2304403: Convert language:weight into a protected property was split off it involved changes to \Drupal\Core\Language\Language::sort() AND it also stalled on the same EntityTranslationTest problems ( see line 476 ).
So #2304403: Convert language:weight into a protected property is a subset of the changes in this issue..that contain the problem.
This issue should be postponed and the other reopened. We can then work on the common problem without
the daily rerolls that blights this issue, Also it should be easier to identify the problem needle in a smaller haystack!
Comment #265
martin107 CreditAttribution: martin107 commentedpostponed correctly
Comment #266
YesCT CreditAttribution: YesCT commented#261 3. is #2299795: Fix migrate NodeRevisionTest & NodeRevisionByNodeTypeTest
Also, noting #2315095: EntityTranslationTest webtest random failure in some patches (not HEAD)
Comment #267
YesCT CreditAttribution: YesCT commentedhad to do this anyway while working on the random failure.
rerolled for
#1498660: Refactor taxonomy entity properties to multilingual
#2308821: Replace FormErrorInterface with $form_state->setErrorByName() and $form_state->setError()
#2313823: Use FormStateInterface for all typehints
I'm not sure if we should do the weight breakout of this... maybe we should postpone this on #2246679: Make Language module's LanguageInterface (to be ConfigurableLanguageInterface) extend Core's LanguageInterface?
But we might have to prove that 2246679 would help with the instability of translation related entity things.
Setting to review so the bot will run on the reroll.
Other versions are posted in #2315095: EntityTranslationTest webtest random failure in some patches (not HEAD)
Comment #269
YesCT CreditAttribution: YesCT commentedrerolled for
#1676910: Rename randomName() to randomMachineName()
#2313157: Optimize admin language detection and make it optional (might want to think about this one)
#2309221: Drupal\Core\Datetime\Date should be renamed to DateFormatter
#2316533: Add getValue/setValue/hasValue and isValueEmpty to FormState
Comment #271
YesCT CreditAttribution: YesCT commentedI'm going to make a new patch tomorrow. :) It will be good. :)
Comment #272
alexpottThe language sort was failing because all the properties are protected thereby all the sorts returned 0 causing php to randomise the array order.
This means that SortArray::sortByWeightAndTitleKey() can be completely removed as this is the only use.
Posting patch since @YesCT and I worked on this together.
Comment #273
alexpottComment #274
jmolivas CreditAttribution: jmolivas commentedI am the TCDrupal sprint working with @YesCT
We noticed @alexpott patch had unrelated change but the interdiff was fine so we apply the inteerdiff
Commented line was removed
Since the only usage of sortByWeightAndTitleKey function on core was removed, we remove the function and the test
------
We wrote a php class and test what was mentioned on #272
In order to run the code use:
This is the result
A review needs to be done to make sure the changes are within the scope of the issue. We thing this is really close to be ready but need a final review.
Comment #275
Chris Dart CreditAttribution: Chris Dart commentedRemoved remaining unused dataprovider public function providerTestSortByWeightAndTitleKey()
I am at the TCDrupal sprint with @YesCT
Comment #276
YesCT CreditAttribution: YesCT commentedI'm going to read through this whole thing, and remove unrelated changes.
Comment #277
YesCT CreditAttribution: YesCT commented#2271005: Rename Language module's LanguageInterface to ConfigurableLanguageInterface and Language to ConfigurableLanguage got in. :)
just a reroll.
Comment #278
YesCT CreditAttribution: YesCT commentedI'm coming back to this and will be re-rolling it now (today).
Comment #279
YesCT CreditAttribution: YesCT commentedrerolled (thankfully the previous patch test results had the commit hash in the log)
#2326891: Convert system_element_info() to Element classes removed some code that had a ->id to ->getId change. that functionality was moved to \Drupal\Core\Render\Element\MachineName in #2305839: Convert hook_element_info() to annotated classes
#2037979: "Current user's language" views filter label is named very misleading removed some code, and also added new direct property usages.
#1966436: Default *content* entity languages are not set for entities created with the API added a direct ->id from a language
#1987882: Convert content_translation routes to a new style controller did some changes to use methods from properties. and moved some code around to ContentTranslationController that needed the changes done there.
there might be other new direct usages... but the testbot should catch them.
Let's see how this does.
(conflict interdiff can be ignored)
Comment #281
YesCT CreditAttribution: YesCT commented#2246679: Make Language module's LanguageInterface (to be ConfigurableLanguageInterface) extend Core's LanguageInterface made ConfigurableLanguageInterface extend LanguageInterface
And this patch adds isLocked() to the languageInterface.
So, either ConfigurableLanguage needs isLocked() or isLocked() should be taken off the LanguageInterface (if it is only implementation for Language).
Since ConfigurableLanguage has a locked property, seems like it makes sense to add isLocked() to ConfigurableLanguage
We will probably have to do the same to ConfigurableLanguage as we are doing to this Language, it will need its properties protected and some clean-up.
#2188675: "Translate" local task always visible, leads to WSOD added some direct access to ->id ... I'll do that next. Here is just the isLocked() addition to ConfigurableLanguage.
Comment #282
YesCT CreditAttribution: YesCT commented1.
this does the direct property accesses from #2188675: "Translate" local task always visible, leads to WSOD
(an interdiff for just that bit)
2.
and #2318817: remove method setId() from core/lib/Drupal/Core/Language/LanguageInterface removed setId from LanguageInterface
so this patch added some ->setId() in some tests...
like
PHP Fatal error: Call to undefined method Mock_Language_44567978::setId() in /var/lib/drupaltestbot/sites/default/files/checkout/core/tests/Drupal/Tests/Core/Utility/TokenTest.php on line 74
this patch tried to change
to
so.. send the id in as an arg to getMock...
public function getMock($originalClassName, $methods = array(), array $arguments = array(), $mockClassName = '', $callOriginalConstructor = true, $callOriginalClone = true, $callAutoload = true, $cloneArguments = false, $callOriginalMethods = false)
check the constructor for Language
which is
so...
becomes
(but timplunkett in irc warned might need to getMock the interface, or getMockBuilder the class)
yeah, a more common pattern that I've seen before... so
3.
I'm not sure what to do with the fails like call to function on none object... will have to look at that still.
Comment #285
YesCT CreditAttribution: YesCT commentedoops. missed a setId() in FormatDateTest.
I'm taking another stab at this now.
Comment #286
YesCT CreditAttribution: YesCT commentedfirst rerolled for #2260121: PHPUnit Tests namespace of modules is ambiguous with regular runtime namespace (+ Simpletest tests)
automatic 3 way merge with no conflicts.
Comment #288
YesCT CreditAttribution: YesCT commentedreroll for
#2288123: Basic authentication broken for non-english sites
#2333907: FormatDateTest incorrectly implying a change to local interface language variable will change global assumed interface language langcode
#2333731: WebTestBase::drupalCreateNode() should not hardcode default values
#2107427: Regression: Language names should display in their native names in the language switcher block
#2335659: Remove FormState ArrayAccess usage from core
#2332739: Remove SortArray::sortByWeightAndTitle
Comment #289
YesCT CreditAttribution: YesCT commentedkeeping in mind #2334763: Tidy up of LanguageInterface - removal of setters and other unnecessary methods starting to do that here.
Comment #291
YesCT CreditAttribution: YesCT commentedtaking out anything that could be considered out of scope
#2337825: Update comment references to the interface for ConfigurableLanguage class due to the ConfigurableLanguageInterface (followup)
#2337827: Language locked is a boolean, use TRUE FALSE not 0
#2337833: return should not have a variable name in LanguageMangagerInterface
not sure what to do with #2334763-9: Tidy up of LanguageInterface - removal of setters and other unnecessary methods so leaving that setDefault() in for now.
similar #2337835: set language->default in ConfigLanguageOverrideWebTest
#2337843: LanguageEditForm take out set name and direction, use create() parameters
#2337847: The negotiated method_id does not need to be sorted on the Language or ConfigurableLanguage object.
#2337853: set default better in LanguageConfigurationElementTest
#2337859: LanguageUrlRewritingTest should not be setting the id of a language
#2337867: set default better in MenuLanguageTest
#2337869: set language weight in EntityTranslationTest better using the constructor or something
#2337871: set language default in TwigTransTest better using the constructor or something
--
#2271363: Remove CacheDecorator and ProcessDecorator removed a file (CachedMockBlockManager) this one added back in, probably from a reroll... took back out.
--
there was a bunch of "use" noise. I dont think those are necessary. and #1989380: Remove unused "use" statements (eventually) will do that (or some similar issue) .. or we could have a separate issue related to Language to tidy that up. but we dont need to do that here.
Comment #293
YesCT CreditAttribution: YesCT commentedwhat?! are these changes needed in this patch? and wouldn't it be ->getId() ... or are these not really language objects? (there are more similar things in locale.module)
Comment #294
YesCT CreditAttribution: YesCT commented#2334763: Tidy up of LanguageInterface - removal of setters and other unnecessary methods got in, so work to do here. (I also need to review the issues that go separated out.)
Comment #295
YesCT CreditAttribution: YesCT commented#2339435: Default no longer needs to be a property on Language or ConfigurableLanguage will also help here.
Comment #296
YesCT CreditAttribution: YesCT commentedlinking #2340571: LanguageInterface needs isLocked method for the locked property. as related.
also, in irc @alexpott mentioned for this issue to really be complete, we should also protect the properties on ConfigurableLanguage.
I'm afraid of expanding the scope to that, but .. if that is what we should do, we will.
@alexpott also mentioned that this issue itself is not a beta deadline.
Comment #297
alexpottYep, this issue needs to protect the properties on the Language and ConfigurableLanguage then we can see that we've successfully converted core to use LangaugeInterface methods.
I don't consider this issue a beta-deadline or target since once #2339435: Default no longer needs to be a property on Language or ConfigurableLanguage, #2337847: The negotiated method_id does not need to be sorted on the Language or ConfigurableLanguage object., #2340571: LanguageInterface needs isLocked method for the locked property. and #2340609: use 'ltr' and 'rtl' for const values instead of 0, 1 so config more readable and diffable are done the interface is complete and correct and the objects don't contain anything unnecessary. Therefore, it is possible to use them completely correctly and have the properties change to protected and not break a contrib module.
It is an important issue for D8MI though since it'll enforce correct usage of the API.
Comment #298
martin107 CreditAttribution: martin107 commentedThe weight changes have been hived off it
#2304403: Convert language:weight into a protected property
and where it seemed natural to expand the scope and convert weight to a protected property...
Also, form a conversation in IRC
it would be nice to hive off all the
- $langcode = $langcode ? $langcode : \Drupal::languageManager()->getCurrentLanguage(LanguageInterface::TYPE_CONTENT)->id;
+ $langcode = $langcode ? $langcode : \Drupal::languageManager()->getCurrentLanguage(LanguageInterface::TYPE_CONTENT)->getId();
changes into a separate issue... to make the review process a breeze
Ok so I have created a new issue.
Comment #299
martin107 CreditAttribution: martin107 commentedI am changing my approach ....
Yesterday I opened an issue to filter out all the get->id() changes from this issue..
because this issue is unworkable, "Will cause commit conflicts" should be read as needs daily reroll just to stay still..
As the getId() changes constitute the bulk of this issue, so #2340667: Protect Drupal\Core\Language\Language::id, and use getId() will need a daily reroll as well, aggh as I found out this morning :(
The solution is hive off the smaller part of this issue, not the big ones and resolve the getId() issue when the monster patch has been cut down in size.
So as weight and getId changes have been hived off so the getName() changes have been hived off .. #2341341: Change public 'name' property access on languages to getName() and add back setName()
Comment #300
martin107 CreditAttribution: martin107 commentedJust making notes for myself in public
Now that all other split off issue are green...
There are more candidates for further dissection.
conversion of $this->locked into $this->isLocked()
conversion of direction from public to private
annotation improvements.
I am opening up a new issue to fix the annotations
Comment #301
mgiffordComment #302
YesCT CreditAttribution: YesCT commentedin #2226533-300: Changes to the Language class due to the LanguageInterface (followup) @martin107 mentioned that direction could be split out. indeed it can. I was surprised I hadn't dont that already. Here it is: #2352029: Convert language:direction into a protected property
Note, some of our separate issues have been committed. yay!
#2340571: LanguageInterface needs isLocked method for the locked property.
#2304403: Convert language:weight into a protected property
Comment #303
YesCT CreditAttribution: YesCT commentedso, the list to focus on right now is:
#2341341: Change public 'name' property access on languages to getName() and add back setName() which looks like it has tricky errors.
#2352029: Convert language:direction into a protected property which has some fails but is small.
#2340667: Protect Drupal\Core\Language\Language::id, and use getId() which looks *really* close to being done, but is large.
Comment #304
YesCT CreditAttribution: YesCT commented#2340667: Protect Drupal\Core\Language\Language::id, and use getId() is fixed.
and #2341341: Change public 'name' property access on languages to getName() and add back setName() has been green recently.
yay!
soo...
I guess it is time to see what changes here might be left.
I'll look now.
Comment #305
YesCT CreditAttribution: YesCT commentedit would be nice to get something committed here, so that people who worked on this over time (but not on other child issues) get recognized.
note #2336177: ConfigurableLanguage cannot be created/removed directly, integrate language_save() and language_delete() took care of some of the changes that were in this monster patch.
Only things I could find left were some typehints.
this patch is from scratch.
I read through the entire old one. no more to do here I think (from that old patch).
but we might want to think a bit on if there is something else to do here.
Comment #306
YesCT CreditAttribution: YesCT commentedreroll. automatic 3 way merge, no conflicts.
This needs a review! And I think we might be done here.
Comment #307
tstoecklerThese don't seem to be used.
Otherwise looks perfect.
Comment #308
tstoecklerLet's see.
No interdiff, I just manually removed that hunk from the patch. (Yes... I'm that lazy!)
Comment #309
tstoecklerAwesome! Thanks @YesCT!
Comment #310
catchCommitted/pushed to 8.0.x, thanks!
Comment #312
YesCT CreditAttribution: YesCT commentedThanks everyone. This was pretty epic.