[Feature] Added support for class initializations asserts.#354
Conversation
… Minor code improvements.
|
Sorry, today is not my day... |
Hey, mate, everything is ok ) 👍 😄 |
| /** | ||
| * Before class instance initialization. | ||
| * | ||
| * @Pointcut\Before("initialization(Go\Tests\TestProject\Application\Main)") |
There was a problem hiding this comment.
It's fragile joinpoint, maybe we shouldn't use it in framework at all? And just leave it as experimental feature with special configuration?
There was a problem hiding this comment.
I do not know about that, however, I know that if we have it in code, we should have way to test it. Right?
| /** | ||
| * After class initialization. | ||
| * | ||
| * @Pointcut\Before("staticinitialization(Go\Tests\TestProject\Application\Main)") |
There was a problem hiding this comment.
Comment name and method name "After" differs from the actual pointcut type, which is "Before"
| } | ||
|
|
||
| $wovenAdvisorIdentifiers = $this->getWovenAdvisorIdentifiers($other->getClass()); | ||
| $reflectionClass = ReflectionClass::create($other->getClass(), $this->configuration); |
There was a problem hiding this comment.
Hmm, what is this? I can't remember ::create() method in the ReflectionClass.
UPD1 Ah, ok, you have introduced new helper class for that. Maybe rename it to more meaningful name to prevent collision with native ReflectionClass and same class from goaop/parser-reflection? I would vote for ProxyClassReflectionHelper or similar.
There was a problem hiding this comment.
Agree, lets make it util class/factory class, that is its purpose.
| use Go\ParserReflection\ReflectionEngine; | ||
| use Go\ParserReflection\ReflectionFile; | ||
|
|
||
| final class ReflectionClass extends BaseReflectionClass |
There was a problem hiding this comment.
Maybe rename it to the ProxyReflectionClass? To prevent collision in names with original ReflectionClass
| $advisorIdentifier, | ||
| $index | ||
| ); | ||
| $constraint = new ClassMemberWovenConstraint($this->configuration); |
There was a problem hiding this comment.
👍 I like how one single constraint class now handles every assertion.
| * | ||
| * @Pointcut\Before("staticinitialization(Go\Tests\TestProject\Application\Main)") | ||
| */ | ||
| public function afterClassStaticInitialization() |
There was a problem hiding this comment.
What about method name itself and method body? ) It's unmatched with doc block and pointcut
There was a problem hiding this comment.
Something is seriously wrong with my brain today... sorry. I am going to close up for today, no use for me to keep working... :D
There was a problem hiding this comment.
Yes, it's always cool to have some time for relax 👍 Don't hurry, this project is maintained for 6 years ) Just keep going slowly
| // noop | ||
| } | ||
|
|
||
| public static function createReflectionClass($className, array $configuration) |
There was a problem hiding this comment.
Doc block for typehint and IDE?
| use Go\ParserReflection\ReflectionEngine; | ||
| use Go\ParserReflection\ReflectionFile; | ||
|
|
||
| final class ProxyClassReflectionHelper |
|
Looks good! |
lisachenko
changed the title
|
BTW, if (null === $wovenAdvisorIdentifiers) { // there are no advisor identifiers
return true;
}
if (!isset($wovenAdvisorIdentifiers[$target])) {
return true;
}
if (!isset($wovenAdvisorIdentifiers[$target][$other->getSubject()])) {
return true;
}can be replaced with one single check if (!isset($wovenAdvisorIdentifiers[$target][$other->getSubject()])) {
return true;
}Need to clean this block later in future PR. |
* origin/2.x: [Feature] Add support for class initializations asserts. (#354)
2 participants
Added support for class initializations asserts. Fixed documentation. Minor code improvements.
No BC breaks.