Skip to content

Fix custom column types not working anymore. #2156

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 22, 2022

Conversation

ndm2
Copy link
Contributor

@ndm2 ndm2 commented Dec 14, 2022

fixes #1810

@ndm2
Copy link
Contributor Author

ndm2 commented Dec 14, 2022

Coverage decrease seems to be a "lie", everything that supposedly decreased, was just as uncovered before.

@dereuromark dereuromark merged commit 76b9dee into 0.x Dec 22, 2022
@dereuromark dereuromark deleted the 0.x-fix-custom-column-types branch December 22, 2022 15:18
@roblperry
Copy link
Contributor

@ndm2 , in testMigrationWithCustomColumnTypes you set the expected limit to null for pgsql.

$limit = 15;
if ($adapter->getAdapterType() === 'pgsql') {
    $limit = null;
}

and

$limit = 30;
if ($adapter->getAdapterType() === 'pgsql') {
    $limit = null;
}

As it turns out PostgresAdaptor.php always fails to read the limit for char and varchar in get columns. I plan on reporting it as a bug and offering a fix, but my fix breaks testMigrationWithCustomColumnTypes due to the carve-outs for pgsql.

Is there some constructive reason that the limit should be null in these cases, or were you simply avoiding the tangent of needing to hunt and fix what I am trying to fix?

Either way, thanks for adding tests while contributing ahead of me. It increases my chances of not doing harm.

@ndm2
Copy link
Contributor Author

ndm2 commented Sep 1, 2023

@roblperry I think I just mixed it up with something else, and mistakenly accepted this as the intended behavior. Looking at the history of how this bug slipped in, there seems to be more incorrect and/or missing coverage :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom data types (data domain) not working (mysql)
3 participants