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: Normalize class names #300 #301

Merged
merged 6 commits into from
Jul 7, 2022
Merged

Conversation

Tofandel
Copy link
Contributor

@Tofandel Tofandel commented Jul 4, 2022

Fixes #300

@Tofandel Tofandel marked this pull request as draft July 4, 2022 21:22
@Tofandel Tofandel marked this pull request as ready for review July 5, 2022 09:27
@greg0ire
Copy link
Member

greg0ire commented Jul 5, 2022

If you are having static analysis issues, consider cloning this for Psalm: phpstan/phpstan#7483

psalm.phpstub Outdated Show resolved Hide resolved
psalm.phpstub Show resolved Hide resolved
@greg0ire greg0ire added the Bug Something isn't working label Jul 6, 2022
@greg0ire
Copy link
Member

greg0ire commented Jul 6, 2022

Note for maintainers or @Tofandel : I think the commits can be squashed.

@greg0ire greg0ire merged commit 51e12c3 into doctrine:3.0.x Jul 7, 2022
@greg0ire
Copy link
Member

greg0ire commented Jul 7, 2022

Thanks @Tofandel !

@writ3it
Copy link

writ3it commented Aug 6, 2022

Hi,

this PR breaks test ClassMetadataFactoryTest::testHasGetMetadataNamespaceSeparatorIsNotNormalized in doctrine/orm:2.12.x.

You can check test results here: https://github.com/doctrine/orm/runs/7698594303?check_suite_focus=true.
Test was implemented here: doctrine/orm@884131e to solve issue described here: doctrine/orm#1750.

@Tofandel could you check this case? What we can do?

@Tofandel
Copy link
Contributor Author

Tofandel commented Aug 6, 2022

The threads are very hard to follow, but to me it seems, as per the last comment, that the linked issue had nothing to do with class normalization, and was related to namespace and a different fix was applied, so it's safe to modify this test to isSame and isTrue instead of isNotSame and isFalse, because the test mentionned was a revert and does nothing to check the issue (unless I didn't understand it in the first place), and only checks for the opposite of what is expected, since \DoctrineGlobal_Article is in fact the same class as DoctrineGlobal_Article, which is an issue

You can even see that the real tests for the issues are marked as

    /**
     * @group DDC-115
     */

But this specific test is not

And that this test was first created in doctrine/orm@120e694
And then the next commit just modify this test to check for the exact opposite, which is not the desired behavior, but at the time it was for "performance concerns"

Given that compared to php 5, php 8 is ~120% faster and that doctrine has a lot more caching now and that this really is such a fast triming operation, it's not a valid argument anymore

Tofandel added a commit to Tofandel/orm that referenced this pull request Aug 6, 2022
@writ3it
Copy link

writ3it commented Aug 6, 2022

Thank you a lot for the explanation and improvement!

Tofandel added a commit to Tofandel/persistence that referenced this pull request Aug 6, 2022
Co-authored-by: Grégoire Paris <postmaster@greg0ire.fr>
Tofandel added a commit to Tofandel/persistence that referenced this pull request Aug 6, 2022
Co-authored-by: Grégoire Paris <postmaster@greg0ire.fr>
Tofandel added a commit to Tofandel/persistence that referenced this pull request Aug 6, 2022
Co-authored-by: Grégoire Paris <postmaster@greg0ire.fr>
Tofandel added a commit to Tofandel/persistence that referenced this pull request Aug 6, 2022
Co-authored-by: Grégoire Paris <postmaster@greg0ire.fr>
writ3it added a commit to writ3it/persistence that referenced this pull request Aug 6, 2022
fix: Normalize class names doctrine#300 (doctrine#301)

Author:  Adrien Foulon
Co-authored-by: Grégoire Paris <postmaster@greg0ire.fr>
Tofandel added a commit to Tofandel/persistence that referenced this pull request Aug 6, 2022
Co-authored-by: Grégoire Paris <postmaster@greg0ire.fr>
Tofandel added a commit to Tofandel/persistence that referenced this pull request Aug 6, 2022
Co-authored-by: Grégoire Paris <postmaster@greg0ire.fr>
writ3it added a commit to writ3it/persistence that referenced this pull request Aug 6, 2022
fix: Normalize class names doctrine#300 (doctrine#301)

Author: Adrien Foulon
Co-authored-by: Adrien Foulon
Co-authored-by: Grégoire Paris <postmaster@greg0ire.fr>
@malarzm malarzm added this to the 3.0.3 milestone Aug 6, 2022
malarzm added a commit that referenced this pull request Aug 6, 2022
fix: Backport Normalize class names #300 (#301)
derrabus added a commit that referenced this pull request Aug 8, 2022
* 2.5.x:
  fix: Backport Normalize class names #300 (#301)
derrabus added a commit that referenced this pull request Oct 13, 2022
* 3.0.x:
  Bump CI workflow
  Allow doctrine/event-manager 2 (#310)
  Bump dev tools and CI workflows (#309)
  Identifier is a list of strings
  fix: Backport Normalize class names #300 (#301)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prepended backslash in class name reloads the full metadata from scratch
5 participants