Skip to content
This repository was archived by the owner on Jan 8, 2020. It is now read-only.

[WIP] - Reuse doctrine common annotations#1581

Closed
Ocramius wants to merge 9 commits into
zendframework:masterfrom
Ocramius:feature/reuse-doctrine-common-annotations
Closed

[WIP] - Reuse doctrine common annotations#1581
Ocramius wants to merge 9 commits into
zendframework:masterfrom
Ocramius:feature/reuse-doctrine-common-annotations

Conversation

@Ocramius

Copy link
Copy Markdown
Member

This PR introduces a dependency to doctrine/common and makes tests depend on composer for autoloading. I also removed some parts of tests/_autoload.php since Zend\Loader\StandardAutoloader can handle it.

If you don't know about annotations, please read http://docs.doctrine-project.org/projects/doctrine-common/en/latest/reference/annotations.html .

Basically, the idea is that the annotation parser in ZF2 can still exists, but doctrine/common (which is not to be confused with ORM) already has a quite well affirmed syntax that could be re-used and covers all use cases I can think of right now. We should suggest end-users to use this "standard" instead of suggesting a new one that will collide hard with what currently Symfony 2 and Flow3 use.

@ezimuel has already suggested a nice approach at doctrine/common#156 to help in the process of making doctrine annotations compatible with other formats that could be used (since its parser doesn't really like ZF2's annotations right now).

I hope everybody will agree that we don't really need a new format for annotations since the one in doctrine/common has been working for everyone since a couple of years.

About tests requiring composer run, @Maks3w suggested to use a constant. Will eventually do that later.

Build Status

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.

import this classes instead of use FQDN

@ralphschindler

Copy link
Copy Markdown
Member

I am an emphatic NO on this one. In fact, we should really have this discussion on the mailing list as its still the more canonical place to discuss these things and keep them around for archival purposes.

@Ocramius

Copy link
Copy Markdown
Member Author

@ralphschindler I'm not asking anyone to merge this right now. I would like to see this discussed Wednesday at the meeting.

@ralphschindler

Copy link
Copy Markdown
Member

Ok, in that case, it might be best to put WIP above the body text. I also think we should have this dicsussion on the mailing list first, IRC only limits this to the core developers, instead of all the ZF stakeholders who participate in the mailing list.

@Ocramius

Copy link
Copy Markdown
Member Author

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.

Our convention is to import the exception namespace, not individual exceptions.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Did that because my IDE was complaining. Will revert :)

@weierophinney

Copy link
Copy Markdown
Member

One other thing: adding a dependency on another project is not as easy as adding it to composer. Technically, we have additional requirements:

  • Need to add it to Pyrus (and I'm not even sure if Doctrine\Common is available via pear/pyrus at this time)
  • The full/minimal tarballs would need to include it
  • We create packages for phpcloud currently, which means they'd need to be updated to include it
  • etc.

Adding 3rd party dependencies is not easy, and should not be taken lightly.

As I've noted in IRC and the ML, I agree with the idea of standardizing our annotation syntax. I disagree with requiring that syntax in order to work with our AnnotationManager (which your patch neatly circumvents). The open question for me is how we can adopt the Doctrine annotation syntax in a way that will work easily across all installation vectors.

@Ocramius

Copy link
Copy Markdown
Member Author

@weierophinney we can simply copy the sources over, as said. No need to make things more complex than they already are. Anyway, a solution for including external libraries is highly NEEDED, and the problem needs some thinking (not a problem for now, but you already noticed what the trend is).

Comment thread tests/_autoload.php

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.

s/Coul/Could/

@Ocramius

Copy link
Copy Markdown
Member Author

@weierophinney should this PR be completed or are you thinking of rewriting the reflection classes on your own?

@weierophinney

Copy link
Copy Markdown
Member

I've got work done already on the annotation manager; I'll likely only use the changes to the annotation classes themselves that you've done. If you want to put those into a separate PR, I'll be able to merge them more cleanly.

@Ocramius

Copy link
Copy Markdown
Member Author

@weierophinney fine by me, I will strip the unrelated commits and rebase the work on your branch :)
Closing here

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants