Skip to content
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

fix: class normalisation test #9966

Merged
merged 3 commits into from
Aug 7, 2022
Merged

fix: class normalisation test #9966

merged 3 commits into from
Aug 7, 2022

Conversation

Tofandel
Copy link
Contributor

@Tofandel Tofandel commented Aug 6, 2022

As per doctrine/persistence#301

This test was not added to fix a bug, but to make sure that classes are not normalized at the time for "performance reasons" this check makes no sense now and in fact causes bugs without the normalization, this was solved in the linked PR and now causes this test to fail

I changed the test to check for the exact opposite as per when it was first introduced https://github.com/doctrine/orm/blame/705a477067e7a68c834ed65605d8d230677b5eaf/tests/Doctrine/Tests/ORM/Mapping/ClassMetadataFactoryTest.php#L62

writ3it
writ3it previously approved these changes Aug 6, 2022
@Tofandel
Copy link
Contributor Author

Tofandel commented Aug 6, 2022

Might need to skip this test on php7.1 as the version of doctrine/persistence with this fix is limited to php 7.2

@writ3it
Copy link

writ3it commented Aug 6, 2022

Tests for 7.1 are using doctrine/persistence:2.5.x which is not using normalization. Class which inherits Abstract ClassMetadataFactory from doctrine/persistence could work in different way depends on php version. This is a little bit weird because doctrine/orm is supporting 7.1-8.1. Alternative solution is introducing normalization in doctrine/persistence:2.5.x.

@Tofandel
Copy link
Contributor Author

Tofandel commented Aug 6, 2022

Backport is there doctrine/persistence#304

derrabus
derrabus previously approved these changes Aug 7, 2022
@derrabus derrabus closed this Aug 7, 2022
@derrabus derrabus reopened this Aug 7, 2022
@derrabus derrabus closed this Aug 7, 2022
@derrabus derrabus reopened this Aug 7, 2022
@derrabus
Copy link
Member

derrabus commented Aug 7, 2022

The low-deps test ist still failing. Let's delete that test. I don't think that we should test the behavior of the persistence library here.

@Tofandel
Copy link
Contributor Author

Tofandel commented Aug 7, 2022

Sure, I agree we shouldn't test class behavior of another package here that leads to this kind of CI pains 😬, the test is in persistence anyways already

But if its failing it just means it still hasn't picked the new version, does it need to be bumped manually or something?

I'm thinking it's being cached by the github action

@Tofandel
Copy link
Contributor Author

Tofandel commented Aug 7, 2022

Yep it's cached based on the commit, so it got cached before the release and it's not trying to upgrade anymore

Cache restored successfully
Cache restored from key: linux-php-7.1.33-47+ubuntu20.04.1+deb.sury.org+1-lowest-b97b176328ff755e64de67a70fffbd8b35ac93958fe87d295c581b2046a136ce

derrabus
derrabus previously approved these changes Aug 7, 2022
@derrabus
Copy link
Member

derrabus commented Aug 7, 2022

PHPCS fails, can you run PHPCBF please? You can ignore the Psalm and PHPStan errors. I'm going to take care of those.

@derrabus derrabus merged commit f7e202f into doctrine:2.12.x Aug 7, 2022
@derrabus derrabus added this to the 2.12.4 milestone Aug 7, 2022
@derrabus
Copy link
Member

derrabus commented Aug 7, 2022

Thanks

derrabus added a commit to derrabus/orm that referenced this pull request Aug 7, 2022
* 2.12.x:
  Fix build (doctrine#9964)
  fix: class normalisation test (doctrine#9966)
derrabus added a commit to derrabus/orm that referenced this pull request Aug 7, 2022
* 2.13.x:
  Address DBAL 3.4 deprecations (doctrine#9969)
  Improve phpdoc for ClassMetadataInfo (doctrine#9965)
  Fix build (doctrine#9964)
  fix: class normalisation test (doctrine#9966)
  Support native enum hydration when using `NEW` operator (doctrine#9936)
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.

None yet

3 participants