Skip to content

Conversation

@puzzledpolymath
Copy link
Contributor

@puzzledpolymath puzzledpolymath commented Jul 10, 2025

Quick fix for ignoring size differences for boolean columns, when matching defaults.

Without this or a similar fix, migrations will continually find unnecessary changes in the schema.

@puzzledpolymath
Copy link
Contributor Author

puzzledpolymath commented Jul 10, 2025

I see you commented out:

        // if (! $result && $this->userType === 'boolean' && $this->size === 1 && $initial->size === 4) {
        //     return true; // Ignore size differences for boolean columns when matching defaults
        // }

I was able to get tests parsing without the code above, however within an application context, using migrations etc changes are continually detected unless a preventative measure like above exists.

What's weird is that the following assertions are valid, E.g. the size is different between the column and schema object.

$this->assertSame(1, $column->getSize());
$this->assertSame(4, $schema->column('column')->getSize());

So with the assertion below, you expect it to return false (without the measures implemented in this PR)??

$this->assertTrue($schema->column('column')->compare($column));

[EDIT]
Ahh I see you did the inverse when comparing. I'll test locally to see whether my findings above are still valid.

$this->assertTrue($column->compare($schema->column('column')));

@puzzledpolymath
Copy link
Contributor Author

Testing locally against this repository, the comparison works fine and I guess that's since MySQLColum.php declares size as a parameter to ignore. So AbstractColumn::compare(self $initial): bool is working correctly.

public const EXCLUDE_FROM_COMPARE = ['size', 'timezone', 'userType', 'attributes', 'first', 'after'];

So perhaps the issue now lies within another repository.

@puzzledpolymath
Copy link
Contributor Author

The issue seems related to defaultValue

image

@puzzledpolymath
Copy link
Contributor Author

puzzledpolymath commented Jul 10, 2025

I'm yet to find a way to reproduce this through a test. In my case the issue is that when initializing the schema and fetching the tables, any boolean/tinyint column has a defaultValue like "0".

At this point in time, there's no data available to change/cast the defaultValue. The column type is a tinyint so the value is treated as one.

During comparison AbstractColumn:compare(self $initial): bool fails because it ends up comparing the default values 0 and false.

I believe the appropriate fix is something like below:

    public function compare(self $initial): bool
    {
        $normalized = clone $initial;

        // soft compare, todo: improve
        if ($this == $normalized) {
            return true;
        }

        $columnVars = \get_object_vars($this);
        $dbColumnVars = \get_object_vars($normalized);

        $difference = [];
        foreach ($columnVars as $name => $value) {
            if (\in_array($name, static::EXCLUDE_FROM_COMPARE, true)) {
                continue;
            }

            if ($name === 'type') {
                // user defined type
                if (!isset($this->mapping[$this->type]) && $this->type === $this->userType) {
                    continue;
                }
            }

            if ($name === 'defaultValue') {
                $defaultValue = $this->getDefaultValue() instanceof FragmentInterface
                    ? $this->getDefaultValue()->__toString()
                    : $this->getDefaultValue();
                $initialDefaultValue = $initial->getDefaultValue() instanceof FragmentInterface
                    ? $initial->getDefaultValue()->__toString()
                    : $initial->getDefaultValue();

                $defaultValue = $this->userType === 'boolean' 
                    ? (bool) $defaultValue
                    : $defaultValue;
                $initialDefaultValue = $this->userType === 'boolean'
                    ? (bool) $initialDefaultValue
                    : $initialDefaultValue;

                //Default values has to compared using type-casted value
                if ($defaultValue != $initialDefaultValue) {
                    $difference[] = $name;
                } elseif (
                    $defaultValue !== $initialDefaultValue
                    && (!\is_object($this->getDefaultValue()) && !\is_object($initial->getDefaultValue()))
                ) {
                    $difference[] = $name;
                }

                continue;
            }

            if ($value !== $dbColumnVars[$name]) {
                $difference[] = $name;
            }
        }
        if (!empty($difference)) {
            $break = true;
        }

        return empty($difference);
    }
image

Comment on lines +55 to +88
public function testBooleanWithProblematicValues(): void
{
$schema = $this->schema('table');

$column = $schema->boolean('target')
->defaultValue(false)
->nullable(false)
->unsigned(true)
->comment('Target comment');

$schema->save();

$this->assertTrue($schema->exists());

$schema = $this->schema('table');
$target = $schema->column('target');

$this->assertSame(1, $column->getSize());
$this->assertSame(4, $target->getSize());
$this->assertFalse($column->isNullable());
$this->assertFalse($target->isNullable());
$this->assertTrue($column->isUnsigned());
$this->assertTrue($target->isUnsigned());

$object = new \ReflectionObject($target);
$property = $object->getProperty('defaultValue');
$property->setAccessible(true);
$defaultValue = $property->getValue($target);

$this->assertSame(false, $column->getDefaultValue());
$this->assertSame(0, $target->getDefaultValue());
$this->assertSame('0', $defaultValue);
$this->assertTrue($column->compare($target));
}
Copy link
Contributor Author

@puzzledpolymath puzzledpolymath Jul 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@roxblnfk It took a while! But here is the test case which confirms the issue, under very specific conditions and attributes defined. Having a comment somehow contributes to the problem.

@cycle cycle deleted a comment from codecov bot Jul 10, 2025
@codecov
Copy link

codecov bot commented Jul 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.41%. Comparing base (f8b3a87) to head (9ac4fb2).
Report is 13 commits behind head on 2.x.

Additional details and impacted files
@@            Coverage Diff            @@
##                2.x     #230   +/-   ##
=========================================
  Coverage     95.41%   95.41%           
- Complexity     1891     1893    +2     
=========================================
  Files           131      131           
  Lines          5273     5279    +6     
=========================================
+ Hits           5031     5037    +6     
  Misses          242      242           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@puzzledpolymath
Copy link
Contributor Author

Hopefully there's not too much push-back or changes required, so this can make it's way into the 2.14.0 release

Made conditional inline
@roxblnfk roxblnfk self-requested a review July 14, 2025 08:52
@puzzledpolymath puzzledpolymath merged commit 5a950e5 into 2.x Jul 14, 2025
32 checks passed
@puzzledpolymath puzzledpolymath deleted the boolean-comparison branch July 14, 2025 09:40
@roxblnfk
Copy link
Member

Please don't merge PRs. We follow conventional commits and need to adhere to certain rules when merging.

@roxblnfk roxblnfk mentioned this pull request Jul 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants