doctrine / orm

Doctrine Object Relational Mapper (ORM)
https://www.doctrine-project.org/projects/orm.html
MIT License
9.86k stars 2.5k forks source link

SchemaValidator: Changing mapping of BIGINT to string|int #11399

Closed ThomasLandauer closed 3 months ago

ThomasLandauer commented 3 months ago

Closes https://github.com/doctrine/orm/issues/11377 Follow-up of https://github.com/doctrine/orm/pull/11396

I just copied the const, so it can be deleted again easily, once DBAL 3 is no longer supported.

greg0ire commented 3 months ago

How can I check DBAL 3 in the test

Same way you did in the code.

Besides that, is the test enough or should I test more scenarios (positive and negative)?

I think a test testing what happens when using just int would be nice to have.

Also, it seems there are CI jobs failing. Please take a look at this guide for more on how to handle those.

ThomasLandauer commented 3 months ago

OK, I also added another test for just string.

What I don't understand: composer install gave me DBAL 4. So where/when is the situation with DBAL 3 ever tested? If the line if (method_exists(BigIntType::class, 'getName')) is not doing what we hope it does (e.g. is always false), then the test will not detect it (since it uses the same logic as the code).

greg0ire commented 3 months ago

This step allows us to test with DBAL 3: https://github.com/doctrine/orm/blob/9c560713925ac5859342e6ff370c4c997acf2fd4/.github/workflows/continuous-integration.yml#L73-L75

thanks to this: https://github.com/doctrine/orm/blob/9c560713925ac5859342e6ff370c4c997acf2fd4/.github/workflows/continuous-integration.yml#L39-L41

If the version detection code is wrong, then the test should fail because it will expect the wrong thing to happen.

greg0ire commented 3 months ago

Pleas squash your commits.

ThomasLandauer commented 3 months ago

OK, done :-)

greg0ire commented 3 months ago

Thanks @ThomasLandauer !

derrabus commented 2 months ago

Thank you very much for your work on this topic, @ThomasLandauer. However, we've decided to revert your patch in favor of #11414.