The 2026 Drupal Business Survey is open. Your response will help shape the future of the Drupal business ecosystem.Problem/Motivation
In \\Drupal\FunctionalJavascriptTests\Core\MachineNameTest::testMachineName there is an if (empty($machine_name_1_value ))-statement a few lines after we assertNotEmpty the exact same variable.
Steps to reproduce
Proposed resolution
Remove redundant if (empty($machine_name_1_value ))-statement, since the variable can never be empty, the assertNotEmpty would already have thrown a ExpectationFailedException and fail the tests if it would be.
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Issue fork drupal-3387556
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:
- 3387556-remove-unneeded-if-statements
changes, plain diff MR !4794
Comments
Comment #3
spokjeComment #4
spokjeComment #5
spokjeComment #6
spokjeComment #7
spokjeComment #8
smustgrave commentedIf I search for in empty\\(\\) always exists and is not falsy find about 39 instances. Should they addressed?
Comment #9
spokjeThey should and will. just not in here.
There are a few smaller ones about "empty not falsy" coming up and after that I am preparing one big issue for the rest of them that have the same, general problem.
These three ones are part of the smaller ones that are specific and don't fit that same, general problem.
Comment #10
smustgrave commentedThanks for clarifying!
LGTM
Comment #11
xjmThis has some exciting merge conflicts.
Comment #12
spokjeComment #13
spokjeComment #14
spokjeComment #15
spokjeComment #16
spokjeComment #17
spokjeComment #18
spokjeTIL:
- Merge conflicts can be exiting
- Downscaling due to 2/3 of the original problem already being fixed elsewhere creates a lot of noise when the person behind keyboard tries to make the IS correct again.
Back to NR.
Comment #19
longwaveNice easy fix here, glad that we have PHPStan to spot such things.
Comment #21
xjmI confirmed the issue reading
core/tests/Drupal/FunctionalJavascriptTests/Core/MachineNameTest.phplocally. Very nice cleanup. In PHPStan we trust!Adding credit for @smustgrave for the scope check. @Spokje's answer makes sense, but it's good to ask the question if the reason for the current scope is not already documented.
Committed to 11.x. I also tried to backport it to 10.1.x since it's a patch-eligible test code cleanup, but there were more merge conflicts in the baseline. (Not exciting ones, just "regenerate the baseline" ones.) That seemed maybe not worth the effort, so leaving fixed against 11.x. Thanks everyone!