IBX-11694: Corrected condition for DisableSiteRootCheckboxIfRootLocationListener

[mateuszdebinski]

🎫 Issue	IBX-11694

Description:

In v5.0, we've shortened this if statement to check the opposite of 4.6, so instead of:

if (null !== $location && 1 >= $location->depth)

It should be:
if (null === $location || 1 < $location->depth)

But it's
if (null === $location || 1 >= $location->depth)

The first part has been correctly changed, while the second part remains the same, which now causes incorrect operation.

Old behaviour is correct, but incorrect we changed it when we moved code to v5.0

[konradoboza]

@mateuszdebinski the PR's title says absolutely nothing about the fix itself, please adjust it to be more informative. 😉

[ViniTou]

But after changes

After what changes?

[Steveb-p]

But after changes

After what changes?

I assume it's upmerge from 4.6 to 5.0 that got changes wrong. @mateuszdebinski ?

[ViniTou]

@Steveb-p
but its still the old one on 4.6 branch.
https://github.com/ibexa/admin-ui/blob/4.6/src/lib/Form/EventListener/DisableSiteRootCheckboxIfRootLocationListener.php#L22

this is 8 years old code, there was no changes to that statement that I can find, and even more this was resolving actual issue at that time. How the fix correspond to old behaviour?

[mateuszdebinski]

@Steveb-p but its still the old one on 4.6 branch. https://github.com/ibexa/admin-ui/blob/4.6/src/lib/Form/EventListener/DisableSiteRootCheckboxIfRootLocationListener.php#L22

this is 8 years old code, there was no changes to that statement that I can find, and even more this was resolving actual issue 8 years ago. How the fix correspond to old behaviour?

In v5.0, we've shortened this if statement to check the opposite of 4.6, so instead of:

if (null !== $location && 1 >= $location->depth)

It should be:
if (null === $location || 1 < $location->depth)

but it's
if (null === $location || 1 >= $location->depth)

But the first part has been correctly changed, while the second part remains the same, which now causes incorrect operation.

Old behaviour is correct, but incorrect we changed it when we moved code to v5.0

[ViniTou]

@mateuszdebinski
ah, now i get it, miss that change from !== to === in history, focused on second part only. Well, not sure why this change was made, anyway 👍, but for the future, that is thr description that should go into the PR.

[alongosz]

Looks like it's a functionality that could use a simple unit test IMHO

[konradoboza]

+1 but I agree with @alongosz. @mateuszdebinski please provide some unit test coverage.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud