Skip to content

Drop base Annotation ancestor#246

Merged
Ocramius merged 1 commit intodoctrine:masterfrom
Majkl578:drop-base-annotation
Feb 26, 2019
Merged

Drop base Annotation ancestor#246
Ocramius merged 1 commit intodoctrine:masterfrom
Majkl578:drop-base-annotation

Conversation

@Majkl578
Copy link
Copy Markdown
Contributor

This PR drops the Doctrine\Annotations\Annotation base annotation class.

It was an inheritance abuse that basically existed only for convenience:

  • to implicitly define default $value property,
  • to behave like stdClass and dynamically set undefined properties (without validation).

* @author Jonathan Wage <jonwage@gmail.com>
* @author Roman Borschel <roman@code-factory.org>
*/
class Annotation
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think the base class should be removed, as it makes the BC break muvh bigger than needed: a deprecation would probably be better

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I was rather thinking about deprecating it in 1.7 and going with less burdens in 2.0. Mostly because this class is rarely used out there. But I may be wrong, GitHub search is not very helpful in finding usages.

Copy link
Copy Markdown
Member

@alcaeus alcaeus Feb 25, 2019

Choose a reason for hiding this comment

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

Deprecating it in 1.7 would be the first step before dropping it from 2.0. We can then see what people say and either decide to revert the deprecation or go along with the removal.

Sneak edit:

GitHub search is not very helpful in finding usages

I checked our application at work and found the following packages extending the annotation class:

  • FOSRestBundle
  • Gedmo Doctrine Extensions
  • Doctrine MongoDB ODM (we already discussed this and will most likely drop it in 2.0)

@Ocramius Ocramius added this to the v2.0.0 milestone Feb 26, 2019
Copy link
Copy Markdown
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

LGTM. The question that remains before merge is if we should stop plans for 1.7.0 and make master 2.0.0 instead. I'd delete the milestone and re-assign everything to 2.0.0 then.

@alcaeus
Copy link
Copy Markdown
Member

alcaeus commented Feb 26, 2019

Master is 2.0.x-dev already.

@Ocramius
Copy link
Copy Markdown
Member

Right, I'm moving all of 1.7.0 stuff into 2.0.0 then.

@alcaeus
Copy link
Copy Markdown
Member

alcaeus commented Feb 26, 2019

Please don’t - 1.7 will be necessary to deprecate features that will be dropped in 2.0 as per SemVer.

@Ocramius
Copy link
Copy Markdown
Member

As per discussion with @alcaeus and @Majkl578, master is already 2.0.0, so this can be merged nao.

@Ocramius Ocramius merged commit ce67a9d into doctrine:master Feb 26, 2019
@Ocramius Ocramius changed the title Drop base Annotation ancestor Drop base Annotation ancestor Feb 26, 2019
@Majkl578 Majkl578 deleted the drop-base-annotation branch February 26, 2019 15:33
@Majkl578 Majkl578 mentioned this pull request Apr 3, 2019
27 tasks
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.

3 participants