-
Notifications
You must be signed in to change notification settings - Fork 64
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
EventsExtension: fix compatibility with Nette 2.4 #100
Conversation
@mrtnzlml It's not so easy, that method does not exist in Nette 2.3. |
Oh, I see. I did not realize that. |
@enumag Fixed. What is the right way to run all tests using tester? I tried this, but without success: |
@@ -313,8 +313,14 @@ private function autowireEvents(Nette\DI\ContainerBuilder $builder) | |||
continue; | |||
} | |||
} | |||
if ($def->getImplementType() === 'get') { | |||
continue; | |||
if (method_exists($def, 'getImplementMode')) { //BC (Nette 2.4) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nette 2.4 is new so "BC" comment is wrong here imho. Also I'd rather put the detection outside of the foreach. It's not really important in compile time but still.
@mrtnzlml I'm not sure I understand - if you want to run all tests, just don't specify any (remove the last argument of your command). |
What do you think about this instead? $implementGetter = method_exists('Nette\DI\ServiceDefinition', 'getImplementMode') ? 'getImplementMode' : 'getImplementType';
...
if ($def->{$implementGetter}() === 'get') { |
@enumag Irrelevant. Same behavior... About your solution - I don't like it, but you are the boss here, so should I change it? |
Dynamic getter is by magnitude worse than method_exists in foreach imho. You can always declare boolean variable outside foreach and do a simple if check inside foreach. |
@Majkl578 I know. It just seemed more readable. @mrtnzlml Actually I'm not the boss here, that's @fprochazka of course. I don't have push rights to this repo. The fastest solution is probably this: $newApi = method_exists('Nette\DI\ServiceDefinition', 'getImplementMode');
...
if (($newApi ? $def->getImplementMode() : $def->getImplementType()) === 'get') { |
I know it's @fprochazka... :) |
Btw there's also new constant for this, $newApi = defined('Nette\DI\ServiceDefinition::IMPLEMENT_MODE_GET');
if ($newApi ? $def->getImplementMode() === $def::IMPLEMENT_MODE_GET : $def->getImplementType() === 'get') |
@Majkl578 Nice. I didn't know about the constant. |
Me neither. I suppose this is faster (maybe)? I am getting tired of rebases. Every solution except the first one worked just fine. Please chose the Chosen One and I'll do rebase (again)... :) |
You don't need rebase, just amend last commit and force push. I don't think it's my call but Majkl's last suggestion is good imo so personally I'd go with that. |
@enumag I am using rebase so often, that I completely forgot about Anyway, amended and green. Still cannot use |
@mrtnzlml Thanks! @fprochazka Ready to merge in my opinion. |
Hmm, tests are indeed a bit strange... But it might be a missing extension in my case (no idea which one though?).
|
@Majkl578 Good point! In my case it's always JSON, so |
Yep, I needed json.so and tokenizer.so. |
@fprochazka Will you please merge it and make new release? |
Thank you guys |
Closes: #99