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

[Feature] Implementation of annotation getters for the ClassField joinpoint (#387) #388

Merged

Conversation

TheCelavi
Copy link
Contributor

  • Allows direct access to class property annotations from aspect
  • Added tests for Go\Aop\Framework\ClassField
  • Added test that verifies that access to annotations exists.

NOTE: Due to singleton implementation of Kernel and access to Reader is done via service locator pattern and static access, it is hard to do a proper testing via injecting mock.

But, at least these tests would detect missing presence of methods which will warn us that we are introducing BC.

@TheCelavi
Copy link
Contributor Author

Instead of those nasty tests, this is much better, we can introduce interface that should provide access to annotations. Much cleaner.

@lisachenko
Copy link
Member

Thanks for PR )

Should we drop support for 2.x? And concentrate support only on new 3.x (aka master)?

Copy link
Member

@lisachenko lisachenko left a comment

Choose a reason for hiding this comment

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

Need to fix years in file docBlocks, all other stuff is correct

/*
* Go! AOP framework
*
* @copyright Copyright 2014, Lisachenko Alexander <[email protected]>
Copy link
Member

Choose a reason for hiding this comment

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

Year isn't correct :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did copy/paste of docBlock.

Different classes have different years, I checked.

To be honest - I am ok for having author and/or copyright annotation, but year in docblock does not have any particular value, we could just remove it - it poses only unnecessary maintenance.

Copy link
Member

Choose a reason for hiding this comment

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

To be honest - I am ok for having author and/or copyright annotation, but year in docblock does not have any particular value, we could just remove it - it poses only unnecessary maintenance.

Looks reasonable. Accepted, will do this for the master branch.

/**
* Provides access to annotations from reflection class/property/method.
*/
interface AnnotationAccess
Copy link
Member

Choose a reason for hiding this comment

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

👍 for interface extraction refactoring

interface AnnotationAccess
{
/**
* Gets a annotation.
Copy link
Member

Choose a reason for hiding this comment

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

Typo: aN annotation, also in the child class


use Go\Aop\Support\AnnotationAccess;

class ClassFieldAccessTest extends \PHPUnit_Framework_TestCase
Copy link
Member

Choose a reason for hiding this comment

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

👍 for the test case

@lisachenko
Copy link
Member

AppVeyor is not working, crappy Windows shit... Couldn't install PHP7.2.1 from Chokolatey recipe due to unknown bug with KB packet.

@TheCelavi
Copy link
Contributor Author

Should we drop support for 2.x? And concentrate support only on new 3.x (aka master)?

PHP 5.x seams dead/dying, and it is easier for us to maintain only one version.

@scrutinizer-notifier
Copy link

The inspection completed: No new issues

@lisachenko lisachenko changed the title Resolved #387 [Feature] Implementation of annotation getters for the ClassField joinpoint (#387) Jan 22, 2018
@lisachenko lisachenko merged commit 8d52f89 into goaop:master Jan 22, 2018
@lisachenko
Copy link
Member

Merged into master, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants