#109 | interdiff.txt | 486 bytes | Anonymous (not verified) |
#109 | 2822837-109.patch | 188.83 KB | Anonymous (not verified) |
|
#107 | 2822837-107.patch | 188.35 KB | Anonymous (not verified) |
|
#101 | interdiff.txt | 632 bytes | Anonymous (not verified) |
#101 | 2822837-101.patch | 189.78 KB | Anonymous (not verified) |
|
#99 | 2822837-99.patch | 189.47 KB | Anonymous (not verified) |
|
#99 | interdiff.txt | 1.29 KB | Anonymous (not verified) |
#96 | interdiff-2822837-86-90.txt | 157.64 KB | chiranjeeb2410 |
#92 | interdiff.txt | 6.39 KB | Anonymous (not verified) |
#92 | 2822837-92.patch | 189.78 KB | Anonymous (not verified) |
|
#90 | 2822837-90.patch | 4.42 KB | chiranjeeb2410 |
|
#86 | interdiff.txt | 8.11 KB | Anonymous (not verified) |
#86 | 2822837-86.patch | 189.02 KB | Anonymous (not verified) |
|
#85 | interdiff.txt | 913 bytes | Anonymous (not verified) |
#85 | 2822837-85.patch | 188.11 KB | Anonymous (not verified) |
|
#82 | interdiff.txt | 1.6 KB | Anonymous (not verified) |
#82 | 2822837-82.patch | 188.1 KB | Anonymous (not verified) |
|
#79 | interdiff-8.3-8.4.txt | 1.09 KB | Anonymous (not verified) |
#79 | 2822837-79-8.4.patch | 187.98 KB | Anonymous (not verified) |
|
#78 | interdiff.txt | 1.69 KB | Anonymous (not verified) |
#78 | 2822837-78.patch | 189.07 KB | Anonymous (not verified) |
|
#74 | bad-test__FormBuilderTest.patch | 2.33 KB | Anonymous (not verified) |
|
#74 | bad-test__all-no-last-cases.patch | 15.95 KB | Anonymous (not verified) |
|
#74 | 2822837-74.patch | 189.04 KB | Anonymous (not verified) |
|
#3 | exception-2822837-3.patch | 3.87 KB | martin107 |
|
#7 | DO-NOT_COMMIT-2822837-7.patch | 2.58 KB | martin107 |
|
#9 | halfMonster.patch | 133.91 KB | martin107 |
|
#11 | myMonster.patch | 159.29 KB | Anonymous (not verified) |
|
#13 | monster2.patch | 159.78 KB | Anonymous (not verified) |
|
#16 | unstoppable_monster.patch | 159.78 KB | Anonymous (not verified) |
|
#18 | green_monster.patch | 159.81 KB | Anonymous (not verified) |
|
#19 | convertExpectedException.php_.txt | 3.74 KB | Anonymous (not verified) |
#19 | interdiff-7-18.txt | 4.13 KB | Anonymous (not verified) |
#25 | prettyMonster-2822837-25.patch | 157.49 KB | martin107 |
|
#26 | blondyMonster-2822837-26.patch | 177.07 KB | Anonymous (not verified) |
|
#26 | ExpectedExceptionMessageRegExp_miniMonster.patch.txt | 2.44 KB | Anonymous (not verified) |
#26 | ConfigTest_testValidateNameException_tailMonster.patch.txt | 546 bytes | Anonymous (not verified) |
#28 | chiterMonster-2822837-28.patch | 177.21 KB | Anonymous (not verified) |
|
#28 | interdiff_Blonde-Chiter_26-28.txt | 5.52 KB | Anonymous (not verified) |
#33 | makeupMonster-2822837-33.patch | 180 KB | Anonymous (not verified) |
|
#33 | interdiff_makeup_chiter-28-33.txt | 31.88 KB | Anonymous (not verified) |
#35 | makeupMonster-2822837-35.patch | 179.96 KB | Anonymous (not verified) |
|
#37 | makeupMonster-2822837-37.patch | 179.95 KB | Anonymous (not verified) |
|
#40 | script.zip | 530.09 KB | Anonymous (not verified) |
#42 | 2822837-42.patch | 185.44 KB | Anonymous (not verified) |
|
#44 | victorianMonster.patch | 187.75 KB | Anonymous (not verified) |
|
#44 | violatorMonster.patch | 184.76 KB | Anonymous (not verified) |
|
#48 | 2822837-48.patch | 187.51 KB | Anonymous (not verified) |
|
#49 | 2822837-49-only-fails.patch | 184.52 KB | Anonymous (not verified) |
|
#51 | manual_fix_request_exception-2822837-51-only-fails.patch | 184.56 KB | Anonymous (not verified) |
|
#53 | red_dwarf_2822837-53-fails-only.patch | 19.1 KB | Anonymous (not verified) |
|
#57 | 2822837-57.patch | 187.54 KB | Anonymous (not verified) |
|
#58 | interdiff-first_line-algo_line.txt | 131.85 KB | Anonymous (not verified) |
#65 | 2822837-65.patch | 187.66 KB | Anonymous (not verified) |
|
#65 | interdiff-57-65.txt | 4.1 KB | Anonymous (not verified) |
#65 | script.php_.txt | 16.38 KB | Anonymous (not verified) |
#70 | prepare_patch_2822837-26.patch.txt | 2.97 KB | Anonymous (not verified) |
#70 | script.php_.txt | 16.4 KB | Anonymous (not verified) |
#70 | main-interdiff-between-patches-65-70.txt | 5.62 KB | Anonymous (not verified) |
#70 | 2822837-70.patch | 189.46 KB | Anonymous (not verified) |
|
Comments
Comment #2
martin107 CreditAttribution: martin107 as a volunteer commentedComment #3
martin107 CreditAttribution: martin107 as a volunteer commentedComment #4
dawehnerInstead of doing that in one single test we should ideally do that across core. Even better would be of course if we would somehow script that particular change.
Comment #5
martin107 CreditAttribution: martin107 as a volunteer commentedI have lots of sympathy with that attitude
When I say over 1000 in the issue summary, it is because I 'grep'ed around in my IDE.
so ... It is going to have to be a script - that does a careful bit of string manipulation.
It is tricky because... even if we break that monster patch into 100 sub issues it is still a big ask for a reviewer to visually check.
So I guess we will work this out some way over the next 5 years.
PS
This patch changes only one file ...but at the time I wrote it - I made sure it covered the whole asset library subsystem.
Comment #6
dawehnerWell, with libraries like https://github.com/grom358/pharborist we could build automatic rewrite tools.
Well, which is basically exactly what you do here :) Over time each individual file will be converted and requires full new attention by the review person/committer. If you have just one patch with one logic change, you just need a single of those attentions, which might be huge, but its still smaller than the sum of a lot of microattentions.
Comment #7
martin107 CreditAttribution: martin107 as a volunteer commentedOk so my first pharborist conversion routine.
It is 70% complete. - the conversion script is posted on github
@dawehner could I ask you to briefly have a scan over it
https://github.com/martinfrances107/convertExceptions/blob/master/conver...
As I only started looking at pharborist today it strikes me that I could be ignoring well all the established high performance patterns of doing things - I could have created something so crappy that it needs rewriting
so the pseudo-code
iterate of over all *Test.php file in core
find every doc-block ( ie starting with /** )
if found extract all @expectedException, and @expectedExceptionMessage
and insert a new "$this->setExpectedException()" command into the start of the method below the doc-block
I have yet to implement
insert a new "$this->setExpectedException()" command into the start of the method below the doc-block
I would be grateful if you could take a brief look and tell me in broad terms If I am on the right track
.... more tomorrow
PS
It looks like I have exposed a brittleness in pharborist - the iteration over core files only works if the following flaws are fixed in core ( see DO-NOT_COMMIT-2822837-7.patch )
for the moment I am getting it to work on a sample test class called test.php
Comment #8
dawehnerThat is amazing!! Thank you for trying to solve things properly. To be honest I just talked in general terms and did not expected that you actually write the script.
Comment #9
martin107 CreditAttribution: martin107 as a volunteer commentedWell, as usual I seem to only be able to complete half the remaining tasks :)
I can see a few flaws :-
the indentation is still not correct.
I have yet to remove the @expectedException and @expcectedMessage tags
We spoke of a monster patch - well this is a "half monster" - I just want to see what testbot will say
more soon.
Comment #11
Anonymous (not verified) CreditAttribution: Anonymous commented@martin107 - nice script! I'm bit changed it. But first - what you think about it, Mr. Bot?)
Comment #13
Anonymous (not verified) CreditAttribution: Anonymous commentedCome on, Bot!)
Comment #15
Anonymous (not verified) CreditAttribution: Anonymous commentedOkey. In my first patch was not correctly placed quotes. But the second patch also failed. What the problem now? Maybe the bot just does not like my monsters?( See you again, Bot. Sorry for spam.
Comment #16
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #18
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #19
Anonymous (not verified) CreditAttribution: Anonymous commentedIt alive!)
Comment #20
martin107 CreditAttribution: martin107 as a volunteer commentedThe monster lives!
@vaplas thank you for moving the script on - it is good to see that yours is much more complete than mine.
Just making notes as I visually scan the patch - it looks like the corner cases are MOSTLY being handled correctly.
1)
File '/non/existent/file' does not exist
becomes
'File \'/non/existent/file\' does not exist'
2)
requires a value for the "$route" argument.
becomes
'requires a value for the "$route" argument.'
3)
Link template 'canonical' for entity type 'apple' must start with a leading slash, the current link template is 'path/to/apple'
becomes
'Link template \'canonical\' for entity type \'apple\' must start with a leading slash, the current link template is \'path/to/apple\''
4)
'requires a value for the "$route" argument.'
becomes
'requires a value for the "$route" argument.'
Which is all good.
BUT - thing go wrong in
/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
is looks like using # as a string delimiter is causing problems these lines
- * @expectedException \Drupal\Core\Extension\InfoParserException
- * @expectedExceptionMessageRegExp #broken\.info\.txt#
are exchanged with
+ $this->setExpectedException('\Drupal\Core\Extension\InfoParserException');
+
so the message is dropped. - I am working on this now
Comment #21
Anonymous (not verified) CreditAttribution: Anonymous commented@martin107, great points! Thank you!
About my mistakes (after refactoring):
1. for pretty message with ' and " we can use something like this:
2.
$method_class->setDocComment($doc_comment);
add new line, i don't know why? But we can fix it like:3. Problem with two different variant Message Tag, not # delimiter. But i don't know what means "RegExp" in this context
Edit: I looked all places, we have only "expectedExceptionMessage" and "expectedExceptionMessageRegExp" (only one file! how did you find it?!).
Comment #22
martin107 CreditAttribution: martin107 as a volunteer commented(1)
Ah I was just making notes as I went along, and trying to develop a good mental map of the things that could go wrong. I was not suggesting that we fix it up ... although the output of your changes does look good.
I did not notice (2) - I will work that into the next script - thank you
We are about to ask that a core committer sits down with a cup of coffee and review the patch .. he/she is going to have a headache at the end.
So when I say "I did a visual scan of the patch" well it is something that takes time :)
I am being pulled away onto something else ... I will work on this over the weekend.
PS regarding your script from #19
I was feeling grumpy that I did not know how to fix the indentation problems I was having I see that you fixed so much more ....but your solution to that is simple elegance..
vaplas++
I have pushed your code into github (and given you credit. )
Thanks again
Comment #23
Anonymous (not verified) CreditAttribution: Anonymous commentedIt's great that you have accepted the challenge and strongly want to complete this issue. I am pleased to help you!
Comment #24
dawehnerYou two are my heroes! Its serious a nice work.
One thing which would be really nice, but is probably quite hard: Move the
setExpectedException
call, nearer/next to the actual called/tested function. We could look at the@covers
annotation and in case there is one, move thesetExpectedException
just the line before that.Comment #25
martin107 CreditAttribution: martin107 as a volunteer commentedFrom #21
1) Pretty print enabled.
2) Now filtering out the second line
3) Its a time management thing ... I want to fix these 3 special cases manually rather than update the script. So I have created a side issue.
From #24 .. thank you :)
I am still thinking about that ... there are plenty of examples such as
FileCopyTest::testNonExistentSourceFile()
where as you say we are going to have to fall back to the existing insert at the top code.
PS the script has a flaw
/core/tests/Drupal/Tests/Component/DependencyInjection/Dumper/OptimizedPhpArrayDumperTest.php
There are some newlines added at the end of the modified doc block
I have pushed all of vaplas work onto github, so the current up to date script is found at
https://github.com/martinfrances107/convertExceptions
( With attribution in the commit log )
Comment #26
Anonymous (not verified) CreditAttribution: Anonymous commented24: Crazy nice to hear this from the man whose commits enough to build Drupal from scratch!
25: Quality work as always!
But it may better to collect all the monsters one place?
For example, I also found a monster (tail of monster) in ConfigTest.php
And now: fix new line + added your miniMonster + my tailMonster + the first attempt to add brains to our monster)
Comment #28
Anonymous (not verified) CreditAttribution: Anonymous commentedOgo! 6/233 places (97 IQ). Not bad. But now I'm blonde check the optimality of his choices).. And bit chit just for green.
Comment #29
dawehnerYou continue to rock. One thing which would make it even better:
this could also be
Comment #30
martin107 CreditAttribution: martin107 as a volunteer commentedthanks vaplas
I have pushed a improved algorithm to
https://github.com/martinfrances107/convertExceptions
It seems to work on an improved test.php
if there are multiple calls to bar then I also say we should fall back to inserting new code at the top
for example
I will work on this some more sunday ..
Comment #32
dawehnerLet's make the title clearer what you two try to do.
Comment #33
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #35
Anonymous (not verified) CreditAttribution: Anonymous commentedMr. Bot you are nudnik! It is hard to fix yourself? Because of you, I got caught to copy-paste). Quick manual correction.
Comment #37
Anonymous (not verified) CreditAttribution: Anonymous commentedI think we are well popularize the setExpectedException practice, due to my ignorance)
Comment #38
Anonymous (not verified) CreditAttribution: Anonymous commentedChanges from #28:
MigrateException::class
done (see lines with "MigrateException::class")@martin107, I changed the code a bit. And I need some time to сlear it. But if you want, I can attach dirty version now.
Comment #39
martin107 CreditAttribution: martin107 as a volunteer commentedThat would be great.
I have a partial fragment, but it is at a very early stage.
I commented on this pharborist issue, if we can, I would like to find a solution that can be pushed upstream.
https://github.com/grom358/pharborist/issues/144
Comment #40
Anonymous (not verified) CreditAttribution: Anonymous commentedYes, it would be great to improve the library pharborist!
I lost a lot of time searching for a beautiful way to add a declaration with the current API, but failed. And I made it very crookedly. There are other unpleasant moments, but maybe I just is not undestand it. Example, not detected method on deep level:
My last patch have typo ('foreach' not found). Script in attached archive fix it.
I was starting to refactor and much stepped aside :( But I hope you can extract necessary points and unite with your work!
Comment #41
martin107 CreditAttribution: martin107 as a volunteer commented@vaplas
thanks, I really like to way things are broken up into manageable functions calls so at the top level this is appropriate.
I am going to be busy next week so my time for open source is limited.
As far as thing go
I think there is a bug in Pahrborist
in the area of RootNode::getUseDeclarations() for which I need to file a bug.
That needs fixing before, the ensure()/import() method from
https://github.com/grom358/pharborist/issues/144
can be implemented [ which would give us the chance to "ensure" that a use statement is present in a single command]
I see you have a lot of time and energy to put into this, and I hope we can coordinate our activities , I will help out where possible, but I think I should unassign myself.
If you want to do more here, I will be around to provide helpful reviews.
Comment #42
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #44
Anonymous (not verified) CreditAttribution: Anonymous commentedFriday - monsters time!
1. Fix problem with several namespaces in one file;
2. Fix problem with adding 'use declaration' in alphabetical order;
3. violatorMonster - experiment how many fails will be if shift position in one line down.
Comment #48
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #49
Anonymous (not verified) CreditAttribution: Anonymous commentedSorry for #44, week without monsters affected :(
Continuation of the experiment
Comment #51
Anonymous (not verified) CreditAttribution: Anonymous commentedI do not understand #49 CI error very well, but I hope this manual change will fix it.
Comment #53
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #54
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #57
Anonymous (not verified) CreditAttribution: Anonymous commented24 fails? But patch has only 22
setExpectedException
:)Ok. If I understand correctly, this patch must have all calls
setExpectedException
near the problem places (#24)Comment #58
Anonymous (not verified) CreditAttribution: Anonymous commentedAt last!) See different between first line and line by algo in attached file.
@martin107, I understand your desire to add heuristics, but it's arguable point for me. And maybe we don't need in artificial intelligence.
Now patch #57 injects all calls near a first problem places, and someone can use of this fact, when will manual solves this issue. I think this is what we need.
Please, suggestions for further work.
And my questions:
1. Does anyone know of a good algorithm of insertion 'use declaration'? Alphabet paste seems not always successful, for example:
2. What better use
or
Thanks!
Comment #59
dawehnerGreat work! I think this is groundbreaking work for further automatic rewrites of Drupal potentially!
One strategy we could apply is to sort of place them okay, and exclude files, where this script doesn't work as expected. These excluded files could then be converted in a follow up manually. Does that make sense?
We are using the first one.
Comment #60
klausiWow, awesome work here! The patch looks almost perfect.
Tiny nitpick:
indentation error
We should also have a change record to inform people to use setExpectedException in the future.
Comment #61
martin107 CreditAttribution: martin107 as a volunteer commentedI am happy to see this issue moving on
Here is a draft change record.
https://www.drupal.org/node/2827160
Comment #62
dawehner@martin107 @vaplas
I just try to understand that a bit better. Is the entire patch autogenerated?
Comment #63
Anonymous (not verified) CreditAttribution: Anonymous commentedMany thanks for pretty words and helps!!!
great catch! I'll fix algorithm indentation with next patch
It autogenerated, but with several manual rules for few special cases + patches.txt from #26). In reality, I think we can simplify the script to the madness, because the vast majority of cases - it is the last line, and we do not need all of that logic, that i stuffed :( I will try to carry out this simplification (saved the current script only for the history).
Comment #64
martin107 CreditAttribution: martin107 as a volunteer commentedComment #65
Anonymous (not verified) CreditAttribution: Anonymous commentedQuick Guide by script:
Comment #66
dawehnerAmazing work!
Comment #67
Wim LeersThis is EPIC! Incredible work!
Comment #68
catchThis looks great, but we should either commit it to both 8.2.x and 8.3.x, or commit it to 8.3.x only around beta/RC. Committing it only to 8.3.x now means more chance of patches not cherry-picking.
We normally do the latter for big scripted changes like this, so marking postponed.
If I'm missing something and this should go into 8.3.x and 8.2.x now then please re-RTBC so we can discuss more though.
Comment #70
Anonymous (not verified) CreditAttribution: Anonymous commentedhot up :)
Comment #71
Anonymous (not verified) CreditAttribution: Anonymous commentedOK.
Comment #72
Anonymous (not verified) CreditAttribution: Anonymous commentedIt is correct time for unpostponed?
Comment #73
xjm@tim.plunkett pointed this out to me. From #68, it looks like @catch intended this to be an RC target.
This is likely to share context lines with #2854529: Fix Drupal.Scope.MethodScope - all methods should have scopes, so let's get that one in first, and then unpostpone this one. Thanks!
Comment #74
Anonymous (not verified) CreditAttribution: Anonymous commented@tim.plunkett, @xjm, thank you very mach!
To facilitate verification, I performed the action like #53 (see bad-test__* patches). That is added code:
i.e. increases all positions by one line (insert
$this->setExpectedException(
after hypothetical exception), and save only cases where line != last line (because it is no need to check). Separate patch forFormBuilderTest
because CI error like #49. Bad patches should fail in all cases if all right.In main patch after script add manual fix indentations:
If all right, I run a script to 8.4.x
Comment #77
klausiNice work!
is this correct? why is the setExpectedException() call not further down?
It doesn't really make sense to use assertSame() here. The @dataProvider on that method also does not make sense because it has only one test set. Anyway, that is out of scope for this issue, so the change is fine as is.
Same here: assertNull() is useless, $arguments is useless.
This does not look correct - where should the exception be thrown? On the last line or the line before? If it is the line before then the last line is useless and should be removed.
Yay, finding bugs in test code with this conversion!
Comment #78
Anonymous (not verified) CreditAttribution: Anonymous commented@klausi, great points!
1, 4 - Mr. Bot also also found no problems with a shift down
BreadcrumbTest
andRowTest
- https://www.drupal.org/pift-ci-job/615076. Strangely enough, I definitely remember that I was putting the rules in this was necessary for\Drupal\Tests\migrate\Unit\RowTest::testSourceFreeze
:) All other tests have fails (with the exception ofRandomTest.php
, but it ok, because has for() and possition corrected).2, 3 - valuable findings! New issue, right?
Comment #79
Anonymous (not verified) CreditAttribution: Anonymous commentedLooks like we have only one different case with patch for 8.4. It just hasn't
/core/modules/migrate/tests/src/Kernel/process/CopyFileTest.php
.Comment #81
Anonymous (not verified) CreditAttribution: Anonymous commentedoops, my mistake. Branches haven't difference. I'm just overlooking #2852242: Merge the two files testing the file_copy process plugin. Hence #78 patch Failed to Apply. And #79 suitable for both branches.
77.2 - #2858325: Clean up DiscoveryTraitTest::testDoGetDefinitionException
77.3 - #2858327: Clean up BreadcrumbTest::testSetLinks
PS. Just for clarity, I was also mistaken when I spoke about CI error with
FormBuilderTest
, if someone is trying to read #74 :)Comment #82
Anonymous (not verified) CreditAttribution: Anonymous commentedTrying to fix 3 coding standards messages with #79 patch.
Comment #83
klausiAnother instance where the assertSame() should be removed in a follow-up.
This should probably have another follow-up. The $expected line will never be reached, right?
This change is wrong, should be on the line right before the last line?
I think my last point is a blocker, otherwise looks good!
There are lots of assert*() function calls that will never be reached because the test case throws the exception before that. But that is not the fault of this issue and we will have to clean that up in follow-ups.
Comment #84
klausiOpened #2859286: Forbid @expectedException @expectedExceptionMessage for Coder to forbid those 2 doc annotations in our coding standards.
Comment #85
Anonymous (not verified) CreditAttribution: Anonymous commented83.3 - nice catch! Done + remove unused variable.
#84 <3
Comment #86
Anonymous (not verified) CreditAttribution: Anonymous commentedSounds like fix of this issue сould be bit better :) I changed the script and now it is trying to automatically get rid of all unnecessary assert*.
Comment #87
klausiHow is this possible? the last line will not throw the exception?
Lol, lol, lol ... congratulations, you found another bug in our test system. This test is never executed! Just executed this test locally in its original form and it is failing (../vendor/bin/phpunit modules/system/tests/modules/accept_header_routing_test/tests/Unit/)
I will open a separate issue for that.
Comment #88
klausiCreated #2859538: system/tests/modules/accept_header_routing_test/tests are in the wrong place, never get executed.
Comment #89
klausiCool, then let's fix all the uncalled asserts() after the exceptions here.
Another instance of a useless assert() after the exception.
Same here, the expects() calls below are never invoked. Which means the exepcts() must be moved before the save() call here.
The assertEquals is never called and should be removed.
same here.
and another useless assertSame().
2 useless assert() calls after the exception
more!
the exception should be thrown after preparing the mock, so should be moved after it. Also there is an assertSame() following which again will never be called.
$expected will never be called, right? Also $parameters is unused.
Comment #90
chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented@klausi,
Uploading latest changes mentioned as in #89. However, didn't deal with the first part of 8.
Please review.
Comment #91
yogeshmpawar@chiranjeeb2410 - can you also add a interdiff, so we can check what changes you have done in current patch
Comment #92
Anonymous (not verified) CreditAttribution: Anonymous commented@klausi!!! #89 Absolutely EPIC clenup! Now all
setExpectedException
inserting before last operation! Looks like bye-bye my algos) Now I understand why you proposed to do it outside of this issue ;)Comment #93
Anonymous (not verified) CreditAttribution: Anonymous commented@chiranjeeb2410, thank you for help! @Yogesh Pawar +1, interdiff would be very helpfull.
Comment #94
yogeshmpawarThanks @vaplas - for the updated patch.
Comment #95
klausi#2548061: Fix Drupal\Tests\accept_header_routing_teste\Unit\Routing\AcceptHeaderMatcherTest::testNoRouteFound got in, so this needs a reroll.
Comment #96
chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented@vaplas, thanks for the updated patch. Applies cleanly and changes are adequate. Also @klausi, +1 for #89.
Uploading previous interdiff as part of procedure.
Comment #97
chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commentedComment #98
klausiWe still need a reroll of #92.
Comment #99
Anonymous (not verified) CreditAttribution: Anonymous commentedReroll of #92. Thanks!
Comment #101
Anonymous (not verified) CreditAttribution: Anonymous commentedoops, sorry.
Comment #102
chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented@vaplas,
Thanks for the reroll. Patch applies cleanly. Changes are met with.
Updating to RTBC.
Comment #103
dawehnerComment #104
alexpottLet's get #2859286: Forbid @expectedException @expectedExceptionMessage done first and then core upgraded to use a version of coder with this in. Otherwise we are certain to regress.
Comment #105
klausiThat is done in Coder and I made a new release. The next step is to get #2861793: Upgrade Coder to 8.2.12 in.
Comment #106
klausiThat got committed, yay!
We need a reroll here + an entry for the new sniff in phpcs.xml.dist.
Comment #107
Anonymous (not verified) CreditAttribution: Anonymous commentedReally!!!
Reroll after #2853823: Explode processor is too strict regarding to the input value.
Comment #108
Mile23Nice. Still needs the changes to
core/phpcs.xml.dist
, however.Comment #109
Anonymous (not verified) CreditAttribution: Anonymous commentedopps))) Thank you, @Mile23!
Comment #110
Anonymous (not verified) CreditAttribution: Anonymous commentedAnd #2861793: Upgrade Coder to 8.2.12 now works for 8.3.x too!
Comment #111
klausiNice! Tested manually that the new sniff triggers correctly if such an annotation is in the code base, so this is good to go.
Comment #112
alexpottCommitted and pushed 4656717 to 8.4.x and c302032 to 8.3.x. Thanks!
Comment #114
Anonymous (not verified) CreditAttribution: Anonymous commentedThank you everyone!!!
PS. Drinking in the middle of the working day of the beginning of the week is not the best idea, but what should I do ?!) Beaver Dance!
Comment #116
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedAdding parent issue because this commit added a DrupalPractice sniff to phpcs.xml.dist