Namespace move BC compatibility layer by greg0ire · Pull Request #71 · doctrine/persistence

greg0ire · 2019-12-06T18:21:44Z

This is just a draft, tell me what you think about the example, and I will squash my commits and apply it to all other classes.
Please tell me what should be done about the tests. I assumed they should use the new names, but tell me if you think previous tests with the old names should be kept in a Legacy subdirectory.

The corresponding commit on master is fed87eb

greg0ire · 2019-12-06T18:22:37Z

lib/Doctrine/Common/NotifyPropertyChanged.php

+        .' Use \Doctrine\Persistence\NotifyPropertyChanged instead.',
+        E_USER_DEPRECATED
+    );
+}


Are you ok with this or do you think line 20 should be enough?

If the DebugClassLoader already triggers a deprecation based on the @deprecated annotation we should skip this to avoid duplicate deprecation messages. Otherwise, I definitely want to keep these in.

// cc @nicolas-grekas

That practice on Symfony is to still add all possible runtime deprecations, because using the DebugClassLoader is optional, especially when using packages standalone.

That clears it up. Thanks Nicolas!

@greg0ire I would suggest we add this deprecation message to all deprecated classes/interfaces.

alcaeus · 2019-12-06T18:24:33Z

Heads up, I’ll push a 1.3.x branch for this to target. We’ll add more compatibility layers in there for the BC breaks in 2.0.

lib/Doctrine/Common/NotifyPropertyChanged.php

alcaeus · 2019-12-06T20:25:57Z

lib/Doctrine/Common/NotifyPropertyChanged.php

+        .' Use \Doctrine\Persistence\NotifyPropertyChanged instead.',
+        E_USER_DEPRECATED
+    );
+}


If the DebugClassLoader already triggers a deprecation based on the @deprecated annotation we should skip this to avoid duplicate deprecation messages. Otherwise, I definitely want to keep these in.

// cc @nicolas-grekas

greg0ire · 2019-12-07T16:37:10Z

Travis fails because of Abstract / Exception on classes that are considered new for phpcs but actually aren't

greg0ire · 2019-12-07T16:38:41Z

I checked that tests that used all class names still work, you can do too by checkout out this PR, then running git checkout origin/1.3.x tests && vendor/bin/phpunit

greg0ire · 2019-12-10T18:19:36Z

I fixed conflicts and reverted the move of PersistentObject… that class is not present on master, thanks @alcaeus for thinking of that!

This has already been done on master, let us do it again, but with a BC layer.

alcaeus · 2019-12-10T19:47:47Z

@greg0ire feel free to add exceptions to phpcs.xml for the current naming errors. These should be taken care of separately - I don't feel comfortable shoving that into 2.0 at this point.

We are not sure yet how this classes will be dealt with.

greg0ire · 2019-12-10T20:30:49Z

Oh right, I assumed the phpcs was enabled on touched/new files only somehow. I pushed a separate commit. Tell me if you meant to have a separate PR

alcaeus · 2019-12-10T20:58:53Z

Thanks @greg0ire!

nicolas-grekas · 2019-12-12T11:09:28Z

Tests on Symfony fail with deprecations after this change so I'm on a PR to fix them.
But it happens that's not possible: the deprecation layer in the PR doesn't work I think.

The issue is that class_alias doesn't work, because if a type hint uses the legacy one, no deprecation will be triggered - and the alias won't ever be loaded - but PHP will fail with a fatal error saying "incompatible type hint".

Fixing this would require loading the alias all the time (and thus removing the deprecation, which is fine if there is no better way...)

alcaeus · 2019-12-12T11:28:04Z

Thanks for the feedback! I’ll try to work on this today.

nicolas-grekas · 2019-12-12T11:52:34Z

See symfony/symfony#34949

@greg0ire

…ersistence (nicolas-grekas) This PR was merged into the 3.4 branch. Discussion ---------- [DoctrineBridge] try to fix deprecations from doctrine/persistence | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - Follows doctrine/persistence#71 But the BC layer is not working yet, as highlighted by the `XXX` in the attached patch. At least for the corresponding interfaces, doctrine/persistence should always alias the legacy name to the new one. /cc @greg0ire @alcaeus FYI Commits ------- 53a4711 [DoctrineBridge] try to fix deprecations from doctrine/persistence

nicolas-grekas · 2019-12-12T12:40:04Z

Actually the aliases are always loaded, my bad. I made 3.4 green. Let's try with upper branches.

A backwards-compatibility layer has been added to persistence to help consumers move to the new namespacing. It is based on class aliases, which means the type declaration changes should not be a BC-break: types are the same. See doctrine/persistence#71

Majkl578 · 2019-12-12T20:14:00Z

Honestly this approach is disgusting as it totally breaks preloading.

Majkl578 · 2019-12-12T20:17:45Z

cc @Ocramius @lcobucci @beberlei @goetas This is serious with PHP 7.4 being out.

A backwards-compatibility layer has been added to persistence to help consumers move to the new namespacing. It is based on class aliases, which means the type declaration changes should not be a BC-break: types are the same. See doctrine/persistence#71

alcaeus · 2019-12-12T20:24:46Z

This is perfectly fine if our aim is to not break the entire ecosystem. If preloading breaks, you’re welcome to create a PR to fix it without removing the deprecation layer. Thanks.

Related to bug #78918.

greg0ire commented Dec 6, 2019

View reviewed changes

alcaeus self-requested a review December 6, 2019 18:24

alcaeus changed the base branch from 1.2.x to 1.3.x December 6, 2019 20:16

alcaeus reviewed Dec 6, 2019

View reviewed changes

alcaeus added the Deprecation label Dec 6, 2019

alcaeus added this to the 1.3.0 milestone Dec 6, 2019

greg0ire force-pushed the namespace-move-bc-compat-layer branch 4 times, most recently from 9a2f323 to a40e414 Compare December 7, 2019 16:33

greg0ire marked this pull request as ready for review December 7, 2019 16:37

greg0ire requested a review from alcaeus December 7, 2019 19:57

greg0ire force-pushed the namespace-move-bc-compat-layer branch 2 times, most recently from a38e3df to c494e68 Compare December 10, 2019 18:18

greg0ire force-pushed the namespace-move-bc-compat-layer branch 2 times, most recently from 10a6d1f to 425d6c0 Compare December 10, 2019 18:35

Move everything to a new Persistence subnamespace

548f75f

This has already been done on master, let us do it again, but with a BC layer.

greg0ire force-pushed the namespace-move-bc-compat-layer branch from 425d6c0 to 548f75f Compare December 10, 2019 18:43

Add exceptions to abstract and exception rules

da3b167

We are not sure yet how this classes will be dealt with.

alcaeus approved these changes Dec 10, 2019

View reviewed changes

alcaeus merged commit 08e7fbd into doctrine:1.3.x Dec 10, 2019

alcaeus self-assigned this Dec 10, 2019

greg0ire deleted the namespace-move-bc-compat-layer branch December 10, 2019 21:07

alcaeus mentioned this pull request Dec 10, 2019

Merge 1.3.x into master #74

Merged

nicolas-grekas mentioned this pull request Dec 12, 2019

[DoctrineBridge] try to fix deprecations from doctrine/persistence symfony/symfony#34949

Merged

greg0ire mentioned this pull request Dec 12, 2019

Address deprecations from persistence doctrine/orm#7953

Merged

doctrinebot mentioned this pull request Dec 12, 2019

Merge release 1.3.0 into master #77

Merged

doctrine locked and limited conversation to collaborators Dec 12, 2019

Majkl578 referenced this pull request in php/php-src Dec 16, 2019

Add support for class_alias to preloading

baf3a91

Related to bug #78918.

Uh oh!

Conversation

greg0ire commented Dec 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

greg0ire Dec 6, 2019

Choose a reason for hiding this comment

Uh oh!

alcaeus Dec 6, 2019

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Dec 6, 2019

Choose a reason for hiding this comment

Uh oh!

alcaeus Dec 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alcaeus commented Dec 6, 2019

Uh oh!

Uh oh!

Uh oh!

alcaeus Dec 6, 2019

Choose a reason for hiding this comment

Uh oh!

greg0ire commented Dec 7, 2019

Uh oh!

greg0ire commented Dec 7, 2019

Uh oh!

greg0ire commented Dec 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alcaeus commented Dec 10, 2019

Uh oh!

greg0ire commented Dec 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alcaeus commented Dec 10, 2019

Uh oh!

nicolas-grekas commented Dec 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alcaeus commented Dec 12, 2019

Uh oh!

nicolas-grekas commented Dec 12, 2019

Uh oh!

nicolas-grekas commented Dec 12, 2019

Uh oh!

Majkl578 commented Dec 12, 2019

Uh oh!

Majkl578 commented Dec 12, 2019

Uh oh!

alcaeus commented Dec 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

greg0ire commented Dec 6, 2019 •

edited

Loading

alcaeus Dec 6, 2019 •

edited

Loading

greg0ire commented Dec 10, 2019 •

edited

Loading

greg0ire commented Dec 10, 2019 •

edited

Loading

nicolas-grekas commented Dec 12, 2019 •

edited

Loading