[WIP]Added Phalcon\Mvc\Model\Binder #12341
Conversation
Jurigag
changed the title
Jurigag
force-pushed
the
3.1.x-binding
branch
15 times, most recently
from
3947a6f to
44cb0ff
Compare
| */ | ||
| public function setModelBinding(boolean value) | ||
| public function setModelBinding(boolean value, var cache = null) |
There was a problem hiding this comment.
deprecated public function setModelBinding(boolean value, var cache = null)| /** | ||
| * Enable model binding during dispatch | ||
| * | ||
| * @param \Phalcon\Mvc\Model\BinderInterface modelBinder |
There was a problem hiding this comment.
Not sure if not needed, zephir-lang/zephir#1331 Does it for 100% correctly generate stubs in this case ?
There was a problem hiding this comment.
This is not Phalcon issue.
| * | ||
| * @param \Phalcon\Mvc\Model\BinderInterface modelBinder | ||
| */ | ||
| public function setModelBinder(<BinderInterface> modelBinder, var cache = null) |
There was a problem hiding this comment.
public function setModelBinder(<BinderInterface> modelBinder, var cache = null) -> <DispatcherInterface>| abstract static function getModelName() -> var; |
|
|
||
| if realHandler instanceof Controller { | ||
| let methodName = handler[1]; | ||
| let bindCacheKey = "binding_" . get_class(realHandler) . "_" . methodName; |
|
|
||
| return this->getParamsFromReflection(handler, params, cacheKey, methodName); | ||
| } | ||
| else { |
There was a problem hiding this comment.
else not needed. just throw Exception if handler is not Closure and methodName is null
|
|
||
| /** | ||
| * Sets cache instance | ||
| * @param \Phalcon\Cache\BackendInterface cache |
| * Sets cache instance | ||
| * @param \Phalcon\Cache\BackendInterface cache | ||
| */ | ||
| public function setCache(<BackendInterface> cache) |
|
|
||
| /** | ||
| * Gets cache instance | ||
| * @return Phalcon\Cache\BackendInterface |
There was a problem hiding this comment.
It's needed, otherwise in generated stubs it would return Phalcon\Mvc\Model\BackendInterface
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Don't see a way to solve it.
There was a problem hiding this comment.
How about this?:
for paramKey, methodParam in methodParams {
let reflectionClass = methodParam->getClass();
if !reflectionClass {
continue;
}
let className = reflectionClass->getName();
if typeof className != "string" {
continue;
}
// ...
}Also, what value could reflectionClass->getName() be if it's not a string?
There was a problem hiding this comment.
Nice idea, thanks Sid :) well i think that we don't even need check type of className, not sure though.
| # [3.1.0](https://github.com/phalcon/cphalcon/releases/tag/v3.1.0) (2016-XX-XX) | ||
| - Added `Phalcon\Mvc\Model\Binder`, class used for binding models to parameters in dispatcher, micro, added `Phalcon\Dispatcher::getBoundModels` and `Phalcon\Mvc\Micro::getBoundModels` to getting bound models | ||
| - Deprecated `Phalcon\Dispatcher::setModelBinding`, use `Phalcon\Dispatcher::setModelBinder` | ||
| - Deprecated `Phalcon\Mvc\Controller\BindModelInterface`, use `Phalcon\Mvc\Model\Binder\BindableInterface` |
There was a problem hiding this comment.
So i should change it to one line ? Or what ?
There was a problem hiding this comment.
how about split this work to 3 pr?
|
Any way to have IntegrationTester and UniTester in one tester method ? :C I don't know why but it trying to execute method from IntegrationTester anyway. |
Jurigag
force-pushed
the
3.1.x-binding
branch
8 times, most recently
from
07edb29 to
789ea1b
Compare
Jurigag
force-pushed
the
3.1.x-binding
branch
from
789ea1b to
bdfb336
Compare
Jurigag
force-pushed
the
3.1.x-binding
branch
5 times, most recently
from
edf337b to
6670663
Compare
|
Finally passed tests, had some weird seg fault. I needed to add some useless variable to store callMethod result and then set it to returnedValue, pretty weird @virgofx can you look here ? If it's related somehow to other try catch problems ? Or some other kind of issue Also DON'T merge it yet, want to refractor somehow tests, also tbh there is no action executing after cache is added so need to do this too. What is even MORE weird that without this returnedValue hack on PHP 7 |
| */ | ||
| public function setModelBinding(boolean value) | ||
| public function setModelBinding(boolean value, var cache = null) -> <Dispatcher> |
There was a problem hiding this comment.
Could you please add PHPDoc with example here
| /** | ||
| * Enable model binding during dispatch | ||
| */ | ||
| public function setModelBinder(<BinderInterface> modelBinder, var cache = null) -> <Dispatcher> |
| @@ -737,6 +748,22 @@ abstract class Dispatcher implements DispatcherInterface, InjectionAwareInterfac | |||
| } | |||
|
|
|||
| /** | |||
| * Returns bound models from binder instance | |||
| */ | |||
| public function getBoundModels() -> array | |||
| /** | ||
| * Sets model binder | ||
| */ | ||
| public function setModelBinder(<BinderInterface> modelBinder, var cache = null) -> <Micro> |
|
I think we need detailed description for some public methods |
|
Yea when there will be time i will add phpdoc examples. |
There was a problem hiding this comment.
I reviewed tests, they look pretty nice. But some small changes still required. I'd recommend you to break long tests into smaller ones but name them better so anyone looking into name of a test (without lookint into its code) would understand what exactly it tests, what feature of a method is that.
| * | ||
| * @param IntegrationTester $I | ||
| */ | ||
| public function testDispatcher(IntegrationTester $I) |
There was a problem hiding this comment.
could you name it in a bit more meaningful way? I'd really like to know what actual functionality is tested
There was a problem hiding this comment.
Well it's in BinderCest so it's obviously testing binding with dispatcher :) I think name is fine.
| $dispatcher->setActionName('multiple'); | ||
| $dispatcher->setParams(['people' => $people->cedula, 'robots' => $robot->id]); | ||
|
|
||
| $dispatcher->dispatch(); |
There was a problem hiding this comment.
this is the second time you perform action. It is a good idea to make test to have 3 steps: arrange, act, assert. You already did assertion in this test, so this can be made as another test.
This is important because smaller tests are easier to read, understand, and fix. If setup part of those tests are similar you can move it to separate method and call it using @before annotation
| $dispatcher->setActionName('view'); | ||
| $dispatcher->setParams(['people' => $people->cedula]); | ||
|
|
||
| $dispatcher->dispatch(); |
| $dispatcher->setActionName('multiple'); | ||
| $dispatcher->setParams(['people' => $people->cedula, 'robots' => $robot->id]); | ||
|
|
||
| $dispatcher->dispatch(); |
| $dispatcher->setParams(['people' => $people->cedula]); | ||
|
|
||
| try { | ||
| $dispatcher->dispatch(); |
There was a problem hiding this comment.
to separate test... Probably it should named like "testDispatcherInstanceException" also you can use something
$I->expectException()
method in this test
| * | ||
| * @param IntegrationTester $I | ||
| */ | ||
| public function testMicroHandler(IntegrationTester $I) |
There was a problem hiding this comment.
public function testMicroHandlerAndModelBinder
anyway I think you can make the test name even more verbose and specific
| $I->assertEquals([ | ||
| 'people' => 'Phalcon\\Test\\Models\\People', | ||
| ], $this->cache->get('_PHMB_/{people}')); | ||
| } |
There was a problem hiding this comment.
yeah, this test can be split into few as well
| * | ||
| * @param IntegrationTester $I | ||
| */ | ||
| public function testMicroController(IntegrationTester $I) |
| $I->assertEquals([ | ||
| 'people' => 'Phalcon\\Test\\Models\\People', | ||
| ], $this->cache->get('_PHMB_Test9Controller_viewAction')); | ||
| } |
There was a problem hiding this comment.
should be split into few tests... each action section should make its test
| $I->assertEquals([ | ||
| 'people' => 'Phalcon\\Test\\Models\\People' | ||
| ], $this->cache->get('_PHMB_Test9Controller_viewAction')); | ||
| } |
|
Yea on weekend i will try to refactor tests. Most of work is done but don't have time to finish it :) |
Jurigag
force-pushed
the
3.1.x-binding
branch
from
6670663 to
72021b7
Compare
|
@DavertMik @sergeyklay something like this tests now ? Well need to clean |
Jurigag
force-pushed
the
3.1.x-binding
branch
3 times, most recently
from
634b121 to
dc4807d
Compare
| @@ -43,25 +47,48 @@ class LazyLoader | |||
| /** | |||
| * Initializes the internal handler, calling functions on it | |||
| * | |||
| * @todo move code to callMethod method in 4.0.0 and remove this method | |||
| +------------------------------------------------------------------------+ | ||
| | Phalcon Framework | | ||
| +------------------------------------------------------------------------+ | ||
| | Copyright (c) 2011-2016 Phalcon Team (https://phalconphp.com) | |
There was a problem hiding this comment.
Could you please fix indents?
| /** | ||
| * Return the model name or models names and parameters keys associated with this class | ||
| */ | ||
| abstract function getModelName() -> string|array; |
| /** | ||
| * Gets cache instance | ||
| */ | ||
| public function setCache(<BackendInterface> cache) -> <Binder> |
Jurigag
force-pushed
the
3.1.x-binding
branch
from
dc4807d to
52c62f7
Compare
Jurigag
force-pushed
the
3.1.x-binding
branch
from
52c62f7 to
e6d887d
Compare
|
I removed this Those tests are fine now ? You meant something like this ? @DavertMik |
|
Thanks |
PR with model binding changes in dispatcher, moving code to separated class, as well add more abilities, caching of reflection, and make binding working for micro handlers, controllers and lazy controllers
If it doesn't merge cleanly with master you may be asked to rebase your changes.
more information.
files directly or those whose functionality/fixes are implemented in the C
programming language