Skip to content

Add Constants Support#263

Closed
jkufner wants to merge 7 commits into
doctrine:1.12.xfrom
jkufner:constants
Closed

Add Constants Support#263
jkufner wants to merge 7 commits into
doctrine:1.12.xfrom
jkufner:constants

Conversation

@jkufner

@jkufner jkufner commented Mar 27, 2019

Copy link
Copy Markdown
Contributor

Continuation of #226. Original commits (excl. merges) cherry-picked onto 1.7 branch.

This pull request adds complete support for constant annotations. It should be the same as property annotations support.

All tests passing.

Example:

class SomeClass {
    /**
     * @SomeAnnotation
     */
    const SOME_CONSTANT = "foo";
}

$classRefl = new ClassReflection(SomeClass::class);
$constantRefl = $classRefl->getReflectionConstant('SOME_CONSTANT');
$annotations = $annotationReader->getConstantAnnotations($constantRefl);
// $annotations == [ new SomeAnnotation() ]

@jkufner jkufner changed the base branch from master to 1.7 March 27, 2019 21:41
@greg0ire

Copy link
Copy Markdown
Member

I think something needs to be done about the commits. Please fixup commits that fix errors into commits that introduce them, and provide messages in english all the time.

Comment thread lib/Doctrine/Common/Annotations/AnnotationReader.php Outdated
Comment thread lib/Doctrine/Common/Annotations/AnnotationReader.php Outdated
Comment thread lib/Doctrine/Common/Annotations/AnnotationReader.php Outdated
Comment thread lib/Doctrine/Common/Annotations/SimpleAnnotationReader.php Outdated
Comment thread lib/Doctrine/Common/Annotations/AnnotationReader.php
Comment thread lib/Doctrine/Common/Annotations/AnnotationReader.php
Comment thread lib/Doctrine/Common/Annotations/FileCacheReader.php
@jkufner jkufner force-pushed the constants branch 2 times, most recently from bc60fc7 to a88d421 Compare March 28, 2019 14:32
@jkufner

jkufner commented Mar 28, 2019

Copy link
Copy Markdown
Contributor Author

Ok, this should do the trick. I've also updated the pull request for 2.0 version (#264).

Comment thread lib/Doctrine/Common/Annotations/Reader.php Outdated
Comment thread lib/Doctrine/Common/Annotations/AnnotationReader.php Outdated
Comment thread lib/Doctrine/Common/Annotations/AnnotationReader.php Outdated
@Majkl578

Copy link
Copy Markdown
Contributor

I think new features for 1.x are out of question at this point when refactoring for 2.0 is already ongoing.

@jkufner

jkufner commented Mar 28, 2019

Copy link
Copy Markdown
Contributor Author

@Majkl578 Well, I've created two pull requests for both versions. So it can go to 1.7 and refactoring of this feature to 2.0 is already done.

The only differences are in Target enum instead of few constants and in removed readers. Moreover, I did not notice any changes in getMethodAnnotations() and getMethodAnnotation() methods, so I guess the refactoring didn't got too far to cause any troubles here.

@alcaeus

alcaeus commented Mar 29, 2019

Copy link
Copy Markdown
Member

I think new features for 1.x are out of question at this point when refactoring for 2.0 is already ongoing.

Disagree: with how unclear the timeline for 2.0 is, it makes sense adding this to 1.x as well. I see no problem with releasing a final 1.x release at the same time as 2.0 is released, similar to how Symfony releases a minor release for the previous major along with a new major version.

@alcaeus alcaeus changed the base branch from 1.7 to master April 1, 2020 11:26
@jkufner

jkufner commented May 3, 2020

Copy link
Copy Markdown
Contributor Author

I've rebased the MR to the current master (v1.10.x). It seems to work fine as before :)

@greg0ire greg0ire changed the base branch from master to 1.12.x January 1, 2021 19:15
@greg0ire

greg0ire commented Jan 1, 2021

Copy link
Copy Markdown
Member

@jkufner can you please fix the new conflicts?

@jkufner

jkufner commented Jan 4, 2021

Copy link
Copy Markdown
Contributor Author

@greg0ire I've rebased it to the 1.12.x branch. I hope it still works, it's been a long time.

@greg0ire

greg0ire commented Jan 6, 2021

Copy link
Copy Markdown
Member

I hope it still works, it's been a long time.

I'm afraid I have bad news for you 😬

@derrabus

derrabus commented Oct 5, 2022

Copy link
Copy Markdown
Member

Closing because we won't add new features to this library.

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.

6 participants