I'm not sure this _is_ a bug in the code, might be a PHP thing, but in ConfigDiffer I noticed the following line (112) is causing me an issue:

<?php
    return $source == $target;
?>

I have two config arrays and in one of them there's a nested array like this:

a => a
b => b
c => 0
d => d

As things stand this is being treated as equal to this one

a => a
b => b
c => c
d => d

If I change the "==" operator to "===" things work correctly, and the two arrays are marked as different.
I can't see any ill-effect on any of my other configurations. Is there any reason why the less strict operator was used?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alfaguru created an issue. See original summary.

alfaguru’s picture

Patch attached.

jhodgdon’s picture

Status: Active » Needs review
Issue tags: +Needs tests

Thanks!

That seems like a reasonable change. Let's set the patch to Needs Review and make sure it doesn't break any of the tests. Then we should also add a test to reproduce the bug you are seeing before committing it to the module.

Can you export your configuration to a .yaml file (both versions) so I can use them to make a test?

jhodgdon’s picture

http://php.net/manual/en/language.operators.comparison.php

If you compare a number with a string or the comparison involves numerical strings, then each string is converted to a number and the comparison performed numerically. These rules also apply to the switch statement. The type conversion does not take place when the comparison is === or !== as this involves comparing the type as well as the value.

So, 'c' == 0 returns TRUE. There you go. I had somehow misplaced this fact in my head.

Anyway, the patch definitely needs to be committed. But, we should also write a more comprehensive unit test for the ConfigDiffer that would verify the normalize/compare is not returning false differences if the incoming arrays are in different orders, and also take care of this test case.

  • jhodgdon committed de1ce3d on 8.x-1.x
    Issue #2863704 by alfaguru, jhodgdon: ConfigDiffer does not catch some...
jhodgdon’s picture

Title: ConfigDiffer equality test » ConfigDiffer doesn't catch some differences
Status: Needs review » Fixed
Issue tags: -Needs tests

OK! I created a unit test for the ConfigDiffer::same() method. I verified it failed as expected with the test case here, and then applied the patch, and it passed. I'll go ahead and commit this with the test.

nedjo’s picture

Nice catch.

AFAIK best practice in PHP is to always use the stricter identical/not identical comparison operators (===, !==) unless the context specifically calls for the type conversion that comes with the more relaxed equals/not equals. But in Drupal we often default to the equals/not equals forms. The coding standards don't address this.

jhodgdon’s picture

I think you are right that === and !== should be a best practice for PHP, and also correct that Drupal often doesn't, which explains (but doesn't excuse) my bad habits. Anyway, this one instance is fixed now, and our compare for differences in ConfigDiffer should be more accurate!

alfaguru’s picture

Thanks for responding so quickly to this issue. There are occasional cases where == is the right operator to use, but mostly it's best avoided.

Status: Fixed » Closed (fixed)

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