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

Deprecate ClassMetadataInfo #6886

Closed
wants to merge 19 commits into from

Conversation

greg0ire
Copy link
Member

@greg0ire greg0ire commented Dec 13, 2017

The deprecation happened 7 years ago: #249

Here is how I proceeded:

  1. I moved all the code back from ClassMetadataInfo to ClassMetadata
  2. I aliased ClassMetadataInfo to ClassMetadata.
  3. I kept a ClassMetadataInfo class definition in an if (\false) statement to provide IDE autocompletion.

@greg0ire greg0ire changed the title Deprecate classmetadatainfo Deprecate ClassMetadataInfo Dec 13, 2017
@greg0ire greg0ire force-pushed the deprecate_classmetadatainfo branch 4 times, most recently from 64d7c45 to 7c60cfc Compare December 13, 2017 20:21
greg0ire added a commit to greg0ire/SonataDoctrineORMAdminBundle that referenced this pull request Dec 13, 2017
greg0ire added a commit to greg0ire/SonataDoctrineORMAdminBundle that referenced this pull request Dec 13, 2017
greg0ire added a commit to greg0ire/SonataDoctrineORMAdminBundle that referenced this pull request Dec 13, 2017
greg0ire added a commit to greg0ire/SonataDoctrineORMAdminBundle that referenced this pull request Dec 13, 2017
greg0ire added a commit to greg0ire/SonataDoctrineORMAdminBundle that referenced this pull request Dec 14, 2017
greg0ire added a commit to greg0ire/SonataDoctrineORMAdminBundle that referenced this pull request Dec 14, 2017
jordisala1991 pushed a commit to sonata-project/SonataDoctrineORMAdminBundle that referenced this pull request Dec 14, 2017
@Majkl578 Majkl578 added this to the 2.7.0 milestone Dec 16, 2017
*/
public function testTheClassIsDeprecated()
{
class_exists(ClassMetadataInfo::class);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should at least has assertTrue(). Maybe instantiating ReflectionClass would be better as it would throw i.e. once CMI BC is dropped.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once CMI BC is dropped, the class will no longer exist, and this will throw too. Even without the assertTrue, right? I mean there is the @expectedDeprecation, and it will no longer be satisfied, I don't know why you want me to add assertTrue. I'll add it but I don't understand what the point is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't matter really, just thought it may be easier to see failed assertion first than no deprecation thrown or whatever that message looks like.

* @group legacy
* @expectedDeprecation Doctrine\ORM\Mapping\ClassMetadataInfo is deprecated since 2.x and will be removed in 3.0. Use Doctrine\ORM\Mapping\ClassMetadata instead.
*/
public function testTheClassIsDeprecated()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return types here and below please

use Doctrine\Common\ClassLoader;
use Doctrine\ORM\Cache\CacheException;
@trigger_error(sprintf(
'%s is deprecated since 2.x and will be removed in 3.0. Use %s instead.',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: Revisit message later, it may be ComponentMetadata instead of ClassMetadata.

* @todo 3.0 Remove this. PersisterHelper should fix it somehow
*/
public function getTypeOfField($fieldName)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this? Should also use ::class.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't: it says "remove this"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment was for:

class_exists('Doctrine\ORM\Mapping\ClassMetadata');

GitHub somehow put it on wrong line, maybe too large diff...

@greg0ire greg0ire force-pushed the deprecate_classmetadatainfo branch 3 times, most recently from e9d8093 to 983000d Compare December 16, 2017 14:06
@Majkl578
Copy link
Contributor

Also please add note to UPGRADE document, thanks.

@greg0ire
Copy link
Member Author

Triggering the build again

@greg0ire greg0ire closed this Dec 17, 2017
@greg0ire greg0ire reopened this Dec 17, 2017
@greg0ire greg0ire force-pushed the deprecate_classmetadatainfo branch 2 times, most recently from 19c55f7 to 6ba3fe2 Compare December 17, 2017 11:27
@greg0ire greg0ire mentioned this pull request Dec 17, 2017
@greg0ire greg0ire force-pushed the deprecate_classmetadatainfo branch from 4f831e9 to d0e70ce Compare December 20, 2017 14:20
@greg0ire
Copy link
Member Author

Rebased

@lcobucci
Copy link
Member

@greg0ire the destination branch should be 2.7

@greg0ire
Copy link
Member Author

Moved the controversial autoloading to another PR, so that this is easy to merge.

@greg0ire greg0ire force-pushed the deprecate_classmetadatainfo branch from 6621e57 to ee7caef Compare January 18, 2018 17:21
@greg0ire
Copy link
Member Author

greg0ire commented Jan 25, 2018

Don't merge, this crashes:

<?php

require 'vendor/autoload.php';

use Doctrine\ORM\Mapping\ClassMetadata;
use Doctrine\ORM\Mapping\ClassMetadataInfo;

class A
{
    public function __construct(ClassMetadataInfo $a)
    {
    }
}

new A(new ClassMetadata('whatever'));

Changing the order of the 2 tests is important because the first one
autoloads CMI
@greg0ire
Copy link
Member Author

greg0ire commented Feb 5, 2018

Forgot to say that it is now "fixed", but the fix is a bit ugly

@lcobucci
Copy link
Member

@greg0ire moving this to 2.8.0 as we have conflicts to address and want to release 2.7.0 ASAP.

@beberlei beberlei modified the milestones: 2.8.0, 2.8.1, 2.8.2 Dec 4, 2020
@beberlei beberlei removed this from the 2.8.2 milestone Dec 13, 2020
@greg0ire greg0ire force-pushed the 2.7 branch 3 times, most recently from e531738 to 66c95a6 Compare December 1, 2021 20:54
@greg0ire greg0ire changed the base branch from 2.7 to 2.10.x December 28, 2021 22:50
@greg0ire greg0ire closed this Mar 22, 2023
@greg0ire greg0ire deleted the deprecate_classmetadatainfo branch March 22, 2023 18:56
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.

8 participants