I noticed a couple calls to phpunit assertEquals which clearly had the "expected" and "actual" parameters reversed, which can lead to confusing failure messages (and accidental propagation of the incorrect form, when other people copy/paste code blocks that look good). Anyway, I did a search and found a few more where obvious strings or integers, etc, were in the "actual" position. This is a very simple issue, but might as well be fixed...

CommentFileSizeAuthor
#2 2879159-2.patch3.92 KBlomo

Issue fork drupal-2879159

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

LoMo created an issue. See original summary.

lomo’s picture

lomo commented
Status: Active » Needs review
StatusFileSize
new 2879159-2.patch3.92 KB

Patch uploaded.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Issue tags: -Testing system

I don't the value of this outweighs the cost of having other patches needing rerolls.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

In addition to #5 (limited value), we should only do this if we can write a phpcs rule to ensure that this behavior stays consistent.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joachim’s picture

joachim commented

> if we can write a phpcs rule to ensure that this behavior stays consistent.

I don't see how we could write a test that properly enforces this. Sometimes $expected is an expression as well as $actual. Best we could do is enforce that $actual is ALWAYS an expression and never a constant value.

But I don't think that should hold this patch up.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Spokje made their first commit to this issue’s fork.

spokje’s picture

spokje
Primary language Dutch
commented
Assigned: Unassigned » spokje
Status: Needs review » Needs work

Stumbled over a PhpStorm warning about the reverse arguments in one 9.3.x file and ran the inspection on the whole of core/.

spokje’s picture

spokje
Primary language Dutch
commented
Category: Bug report » Task
Issue tags: +Coding standards
spokje’s picture

spokje
Primary language Dutch
commented

Hidden 2879159-2.patch.

Spokje opened merge request !704

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs work » Needs review
spokje’s picture

spokje
Primary language Dutch
commented
Assigned: spokje » Unassigned
quietone’s picture

quietone commented

@Spokje, what inspection are you referring to in #15? Is it just available with PhpStorm?

spokje’s picture

spokje
Primary language Dutch
commented

@quietone This is the PHPStorm inspection I referred to: https://www.jetbrains.com/help/phpstorm/php-misordered-assertequals-asse...

quietone’s picture

quietone commented

Ah, so no way to prevent this from happening again in core?

spokje’s picture

spokje
Primary language Dutch
commented

@quietone: Nope, looks like that PHPStorm inspection is doing exactly what @joachim stated in #18 (emphasis mine)

I don't see how we could write a test that properly enforces this. Sometimes $expected is an expression as well as $actual. Best we could do is enforce that $actual is ALWAYS an expression and never a constant value.

But I don't think that should hold this patch up.

And I fully agree with the last line of the quote.

Basically: Waiting on a new sniff would take time, and the sniff will never be perfect.

quietone’s picture

quietone commented
Assigned: Unassigned » quietone

@Spokje, my apologies. I should have read all the comments.

Assigning to myself for review. Hope to do it in the next few days.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs review » Needs work
quietone’s picture

quietone commented
Assigned: quietone » Unassigned

mondrake did a review before I could.

ravi.shankar’s picture

ravi.shankar commented

Working on above feedbacks.

mondrake’s picture

mondrake
Location 🇮🇹
commented

@quietone I stepped on your toes, sorry

ravi.shankar’s picture

ravi.shankar commented
Status: Needs work » Needs review

@mondrake, please review if this is fine.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs review » Reviewed & tested by the community

Housekeeping this is a bit like Penelope's shroud, but all the changes here seem OK to me. Till the next time...

spokje’s picture

spokje
Primary language Dutch
commented

Thanks @mondrake

Housekeeping this is a bit like Penelope's shroud,

Totally agree, but there's really no sniff (I can think of) that will catch-'em-all. So I fear/think this is the best we can run with at this point in time.

  • catch committed e48dcff on 9.3.x 
    Issue #2879159 by Spokje, LoMo, ravi.shankar, quietone, mondrake: Some...

  • catch committed 76a1fb8 on 9.2.x 
    Issue #2879159 by Spokje, LoMo, ravi.shankar, quietone, mondrake: Some...
catch’s picture

catch
he/him
Primary language English
commented
Version: 9.3.x-dev » 9.2.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 9.3.x and cherry-picked to 9.2.x, thanks!

Spokje closed merge request !704

Status: Fixed » Closed (fixed)

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