Problem/Motivation
#2151223: Add a method to SortArray to sort by weight and title added the sortByWeightAndTitleKey() method, with phpunit coverage testing the comparison return values.
but
made the Core\Language\Language::sort() use it, and did not test actual language objects (importantly where weight was a tie to try and force it to use the title comparison)
Language does not have a title property. It has a name property.
Proposed resolution
Dont use the generic sort.Have language implement the ArrayAccess- fix the sort to use the name property
Remaining tasks
- (done) write a phpunit test for Language::sort() that uses language objects.
- (done) think about how the test should actually be.
- (done) review
User interface changes
No.
API changes
No.
Comment | File | Size | Author |
---|---|---|---|
#50 | 2304651.50.patch | 3.61 KB | YesCT |
#50 | 2304651.50.just-tests.patch | 2.82 KB | YesCT |
#50 | interdiff.2304651.46.50.txt | 773 bytes | YesCT |
#46 | interdiff.2304651.43.46.txt | 978 bytes | YesCT |
#46 | 2304651.46.patch | 3.77 KB | YesCT |
Comments
Comment #1
YesCT CreditAttribution: YesCT commentedtalking with Berdir in irc...
the things to sort get casted to array, so they might have a 'name' key, but they definitely do not have a title one.
so I'm still thinking we are lacking a test that has sorts languages with same weights.
guessing LanguageUnitTest is the place to add a sort test.
Comment #2
YesCT CreditAttribution: YesCT commentedComment #3
YesCT CreditAttribution: YesCT commentedI'm sure this isn't the "right" way for this test to be... but it fails. :)
Comment #4
YesCT CreditAttribution: YesCT commentedguess next step is to try and fix the bug with something like #2151223-20: Add a method to SortArray to sort by weight and title
telling it to use the name (instead of the default title)
Comment #6
YesCT CreditAttribution: YesCT commenteda test that uses a data provider.
( looked at http://phpunit.de/manual/3.7/en/writing-tests-for-phpunit.html#writing-t...
and the example in SortArrayTest )
3 passes, 1 fail.
4 tests, for covering more cases than just the one where weight is tied and label was ignored.
Comment #8
martin107 CreditAttribution: martin107 commentedThe much needed test make sense and look clean and elegant.
But I think the title of this issue could do with some work.
Please note that with my patch the failing test still FAILS so I provided a incomplete solution ... just a nudge in the right direction.
I should reason this out in public in order to give confidence in my solution.
Language::sort() could easily be altered using an anonymous function to override the sortByWeightAndTitleKey function to use NAME not TITLE :-
But that is not the issue. The underlying thing is that $a and $b as fed from the language array are language objects sortByWeightAndTitle() needs a array of key-values pairs.
in short the weight and name are available as $a->weight and $a->name and sortByWeightAndTitleKey needs $a['weight'] and $b['name'].
( I have verified this is a debugger just to be sure )
This misunderstanding was cleared up in #2226533: Changes to the Language class due to the LanguageInterface (followup) which has stalled and is currently being scrapped for parts. So here is the relevant code from there.
In the spirit of fail early and in public ... I am sure there is just something obviously wrong and so I am posting early :-)
PS Regarding the Drupal\system\Tests\Entity\EntityQueryTest fails looks like #2254011: Drupal\system\Tests\Entity\EntityQueryTest random failure is shadowing any changes to the language subsystem. Nuts!
Comment #10
YesCT CreditAttribution: YesCT commented@martin107 sounds like you are thinking the same thing I was.
The original title was: Core Language sort() should not work. sortByWeightAndTitleKey is only for things that implement the \ArrayAccess Interface
because sortByWeightAndTitleKey and the array sorts it calls, take arrays as input.
But in #1, I tried to pass on what Berdir said: that because (there is no type on the parameters in the function definition and) the language objects are cast as arrays, the language objects are some kind of array in the function, and php makes array elements keyed by the property names. (I have not verified with a debugger...)
I'm going to try wrapping and telling it to use the 'name' key.
Comment #11
YesCT CreditAttribution: YesCT commentedsigh. this fails too, and I had hoped it would pass.
Interdiff to 6.
[edit: for anyone following along, this runs just this one test]
./vendor/bin/phpunit --filter=testSortArrayOfLanguages
Comment #12
YesCT CreditAttribution: YesCT commenteduh.. data set #3 is the one that is failing...
--- Expected Other LanguageUnitTest.php 0 Drupal\Tests\Core\Language\LanguageUnitTest->testSortArrayOfLanguages with data set #3()
that is not the one I thought would be failing.
I think I didn't write the tests correctly... cause after "fixing" it, the fails are on the data sets where I thought they were already in order.
Comment #13
YesCT CreditAttribution: YesCT commentedchanged the test to be an array keyed by langcode...
and now the tests pass without the fix. :(
Comment #14
YesCT CreditAttribution: YesCT commentedclearly I'm doing something wrong.
Cause when I change the order of my expected...
the tests still pass.
Comment #18
YesCT CreditAttribution: YesCT commentedUGH. I think the test trying to use the data provider is comparing two empty arrays. and that is why it is passing.
Comment #19
YesCT CreditAttribution: YesCT commentedok. I have two things to verify... that any test is working, and how to get the provider to work.
Here I went back to the non-provider to see if I can tell if two arrays are in different orders.
The test can... when calling sort though, the one that starts in the wrong order gets sorted correct, and the one that starts in the correct order gets sorted wrong.
Which is fine.. if sort is broke in that way... and I can get sort to work as expected.
testDirectSortArrayOfLanguagesWrong()
testDirectSortArrayOfLanguagesCorrect()
are the only two tests doing what I expect them to.
trying that now.
Note this patch has a bunch of debugging stuffs in it.
No interdiff cause it's messy anyway.
With the fix, both the correct and wrong case pass.
Comment #20
YesCT CreditAttribution: YesCT commentedin my provider I was setting the array wrong, with brackets around the index. :/
testSortArrayOfLanguages() is only giving me one fail right now without the fix...
[edit: that is ok. the data is: in order with different weights, out of order with different weights.. and those two should work with the current sort.
then the other two tests have weight tied, so it should force a sort on the title/name]
I think now, I might think about how this should actually be tested.
Comment #21
YesCT CreditAttribution: YesCT commentedfiles this time.
Comment #22
YesCT CreditAttribution: YesCT commentedupdated issue summary.
Comment #23
YesCT CreditAttribution: YesCT commentedthis one takes out the direct test (no phpunit data provider).
So now there is just one new test:
testSortArrayOfLanguages()
and the dataprovider for it.
Comment #29
YesCT CreditAttribution: YesCT commentedok. #23 is looking pretty good.
next step is to try making a patch that runs the entity query test 50 time.... without uploading 50 patches and running the full test suite each time.
Making a note of how to do that.... (thanks @sun)
In run-tests.sh, replace the line $test_list = simpletest_script_get_test_list(); with $test_list = array_fill(0, 50, 'Drupal\Test\To\Repeat');
Comment #30
martin107 CreditAttribution: martin107 commentedPLEASE DO NOT COMMIT
Taking 2304651.23.withfix.patch and altering the run-tes.sh file to run 50 times as per #29
locally first 10 runs all pass.
@YesCT sorry could not follow along until now my week went sideways - but I agree now looking pretty good.
Comment #31
YesCT CreditAttribution: YesCT commentedWe did the repeated test to make sure we are not introducing something like #2254011: Drupal\system\Tests\Entity\EntityQueryTest random failure in head.
Comment #32
penyaskitoIsn't this a dupe of #2239399: Languages should be sorted by label instead of title?
Comment #33
martin107 CreditAttribution: martin107 commented@penyaskito yes they appear to be almost identical in terms of changes to Drupal\Core\Language\Language, So yes it looks like a dup.
Now in terms of which one to advance...
Can I suggest this one should be committed.
A) It overrides 'name' not 'label which is think is the more correct thing to do, as in
B) It has proved that the instability is resolved. 2304651.23.withfix.patch is the active patch and is ready to move to RTBC.
C) While I can see the test from the other issue, there are no comments and it is hard at a glance to see exactly what is covered and what is not.
YesCT comments show a systematic covering of all bases, and this gives me more confidence.
Comment #34
YesCT CreditAttribution: YesCT commented@penyaskito wow! it has been a while since I looked at that whole patch on that issue!
I'm not totally sure it's a complete dup.
I thought the other was changing it to sort by label.
Which still might change what this is doing.
Hopefully, this will just get head doing what it should have, and then we can return to #2239399: Languages should be sorted by label instead of title
I'm still perplexed as to why there were not random fails in the run of 50 tests above...
*maybe* this *fixes* the random fails?
Comment #35
Gábor HojtsyComment #36
penyaskitoIf we don't have random failures as demoed by #30 and we have proper tests, then let's get this in and unblock the issues depending on this one.
Comment #37
penyaskitoRTBC is for #23. @YesCT or @martin107, could you please upload the files again avoiding any potential mistake?
Comment #38
martin107 CreditAttribution: martin107 commentedSure, just to be safe this is a copy of 2304651.23.withfix.patch
I have just given it a quick visual scan to double check.
Comment #39
alexpottThis looks a little complex. I think we can replace with
$this->assertSame($expected, array_keys($languages));
which will also allow for testing sorting more than 2 languages at a time.Comment #40
YesCT CreditAttribution: YesCT commentedI'll do that now.
Comment #41
YesCT CreditAttribution: YesCT commentedran the phpunit test locally, and without the fix, 3 passes and 1 fail, with the fix, 4 passes.
same as in #38, fail is in data set #2. where the they are out of order by weight.
I expected head to pass on that one, and to fail on one of the tests sorting by title (name).
Comment #42
YesCT CreditAttribution: YesCT commentedinterdiff.
Comment #43
YesCT CreditAttribution: YesCT commentedoh, good. it was the third data set that was failing (which is data set #2, since it starts counting at zero. Thanks @alexpott).
that is the set where the language title/name was *in order*. Note this means it was reordering the one that was out of order... this is because it was tieing on title (cause there was no title) and ties have unpredictable behavior.
phew.
Updated my comments so they start counting at dataset #0.
Note also that --tap is helpful (Thanks again @alexpott)
so (for those new to phpunit, and for my future self) I ran the test with:
./vendor/bin/phpunit --filter=testSortArrayOfLanguages
and
./vendor/bin/phpunit --filter=testSortArrayOfLanguages --tap
with sanity restored, I think this is ready.
Comment #45
tim.plunkettThis should have
@covers ::sort
While you're rerolling, can you put a blank line in between these?
Or, you could follow the pattern of ConditionAccessResolverTraitTest::providerTestResolveConditions(), and use $data = array(); $data[] = array(...); return $data;
And then you can instantiate the $languageX object in line with their test case...
Comment #46
YesCT CreditAttribution: YesCT commentedthanks! done.
Comment #47
tim.plunkettThanks!
Comment #48
alexpottSo lets change this to the assertSame (as in #39) so if we add more than 2 languages to the data set it'll still work.
Comment #49
YesCT CreditAttribution: YesCT commentedsorry. looks like I lost the change from 41.
Comment #50
YesCT CreditAttribution: YesCT commentedComment #52
tim.plunkettAhhh, whoops. Nice catch @alexpott.
Comment #53
YesCT CreditAttribution: YesCT commentedComment #54
alexpottCommitted 9bbfede and pushed to 8.x. Thanks!
Comment #56
YesCT CreditAttribution: YesCT commentedgreat! thanks.
Moving to #2239399: Languages should be sorted by label instead of title