Skip to content

Drop SimpleAnnotationReader#199

Merged
Majkl578 merged 1 commit intodoctrine:masterfrom
Majkl578:drop-SimpleAnnotationReader
May 12, 2018
Merged

Drop SimpleAnnotationReader#199
Majkl578 merged 1 commit intodoctrine:masterfrom
Majkl578:drop-SimpleAnnotationReader

Conversation

@Majkl578
Copy link
Copy Markdown
Contributor

@Majkl578 Majkl578 commented May 7, 2018

closes #87

@Majkl578 Majkl578 added this to the v2.0.0 milestone May 7, 2018
@Ocramius
Copy link
Copy Markdown
Member

Ocramius commented May 8, 2018

Question: do we want to enforce importing annotations (as in "stop looking for same-namespace annotations") to remove the ambiguity in resolved symbols? That was the biggest problem with this reader: too much implicit resolution.

@Majkl578 Majkl578 force-pushed the drop-SimpleAnnotationReader branch from 288062e to 6b2f430 Compare May 8, 2018 14:19
@Majkl578 Majkl578 force-pushed the drop-SimpleAnnotationReader branch from 6b2f430 to 9e43569 Compare May 10, 2018 15:56
@Majkl578
Copy link
Copy Markdown
Contributor Author

stop looking for same-namespace annotations

This may be a bit problematic since same-namespace uses are considered superfluous by IDEs and CS.

Honestly I have little experience with this reader in real world since it was always too magical. What I hate most on this reader is the fact that you can register multiple namespaces in which to look for annotations. Then comes the WTF - colliding names are ignored, first one is used:

if ($this->namespaces) {
foreach ($this->namespaces as $namespace) {
if ($this->classExists($namespace.'\\'.$name)) {
$name = $namespace.'\\'.$name;
$found = true;
break;
}
}

Do we have any statistics for SimpleAnnotationReader usage? Would this even be reason to keep it?
Imho imported annotations should stay, same-namespace lookup should stay too (for the reason above), but namespaces to search in should be dropped.

@Majkl578 Majkl578 requested review from Ocramius and lcobucci May 11, 2018 18:12
@Ocramius
Copy link
Copy Markdown
Member

same-namespace uses are considered superfluous by IDEs and CS.

Agreed. I'm mostly talking from experience in dealing with bug reports, where people type in something like @Collection and have a Collection in their current namespace.

This is the scope of my original question.

On the other side, this is consistent with PHP's import and name resolution semantics, so I may just be going nowhere with it.

you can register multiple namespaces

No more: burn it with napalm.

Do we have any statistics for SimpleAnnotationReader usage? Would this even be reason to keep it?

We made the mistake of basing the ORM simplistic setup on it (including many of the docs). The replacement of the annotations can be automated (or at least reported), so we don't need to concern ourselves with it. Let's just kill it.

Given a SimpleAnnotationReader, we can build a tool that reports replacements to be performed.

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.

I think there's nothing else to be done here, and this is good to go from my perspective 👍

@Majkl578
Copy link
Copy Markdown
Contributor Author

Good thing is that it's already gone from ORM dev-master. 🎉

Following PHP resolution rules is the way to go, the least confusing one.
Once we have 2.0 ready for alpha/beta, we can run some survey to see whether missing SimpleAnnotationReader behavior is a blocker for someone or not.

Given a SimpleAnnotationReader, we can build a tool that reports replacements to be performed.

Sounds good to me. 👍

@Majkl578
Copy link
Copy Markdown
Contributor Author

Alright, let's move on. :shipit:

@Majkl578 Majkl578 merged commit b5d5f9e into doctrine:master May 12, 2018
@Majkl578 Majkl578 deleted the drop-SimpleAnnotationReader branch May 12, 2018 15:36
@Majkl578 Majkl578 self-assigned this May 12, 2018
@Majkl578 Majkl578 mentioned this pull request Apr 3, 2019
27 tasks
@alcaeus alcaeus mentioned this pull request May 8, 2020
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.

DCOM-295: Deprecate the SimpleAnnotationReader

2 participants