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

Load model in dispatcher if type hinted in controller action #11288

Merged
merged 9 commits into from Jan 28, 2016

Conversation

Projects
None yet
7 participants
@mattvb91
Contributor

mattvb91 commented Jan 9, 2016

Issue: #11267

TODO:

  • Update changelog
  • Implement tests - Can someone give some advice?

Just a few other notes that I need some help with:

Is there a zephir equivalent of Class::class to get a classname? I dont like the way I have it here defining the classes in strings?

Not sure about the naming of "BindModelInterface" under Phalcon\Mvc\Controller. Any suggestions?

Can someone give me an idea what the best approach is for testing this? Or point out an already existing test class that I can add into.


This allows for typehinting models in the controller actions which will then be loaded for you in the dispatcher:

    public function viewAction(Article $article)
    {
        //$article is already instantiated
    }

It also adds a \Phalcon\Mvc\Controller\BindModelInterface which allows you to set the controllers associated model:

static function getModelName()
{
     return Article::class;
}

This is useful in cases where you have a BaseController that handles a certain action for you but still want to be able to have the model loaded:

BaseController:

public function viewAction(Model $model)
{
      //Will check the child controller to "getModelName()" to load the correct entity
}

Currently this will only work based off /route/action/{id}. Something for a next release would be to allow modifying the table column that it performs the select on instead of ID.

@Jurigag

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Jan 10, 2016

Member

Test case:
Create some route, handle it, and check if parameter in controller is instanceof class ?

Is'nt SomeClass::class a constant ? Maybe try constant("Model::class") but its still doesnt look nice.

Member

Jurigag commented Jan 10, 2016

Test case:
Create some route, handle it, and check if parameter in controller is instanceof class ?

Is'nt SomeClass::class a constant ? Maybe try constant("Model::class") but its still doesnt look nice.

@sergeyklay

This comment has been minimized.

Show comment
Hide comment
@sergeyklay

sergeyklay Jan 10, 2016

Member

SomeClass::class is php 5.5 syntax. We still work with php 5.4

Member

sergeyklay commented Jan 10, 2016

SomeClass::class is php 5.5 syntax. We still work with php 5.4

@mattvb91

This comment has been minimized.

Show comment
Hide comment
@mattvb91

mattvb91 Jan 10, 2016

Contributor

Updated change log and added tests.

If I do a "git merge upstream/2.1.x" so I can resolve the conflict that wont mess up this pull request will it?

Contributor

mattvb91 commented Jan 10, 2016

Updated change log and added tests.

If I do a "git merge upstream/2.1.x" so I can resolve the conflict that wont mess up this pull request will it?

@Jurigag

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Jan 11, 2016

Member

Yea, just pull changes from upstream/2.1.x and merge them. Also squash your commits to one.

Member

Jurigag commented Jan 11, 2016

Yea, just pull changes from upstream/2.1.x and merge them. Also squash your commits to one.

Updated changelog/fixed conflict
Conflicts:
	CHANGELOG.md
@mattvb91

This comment has been minimized.

Show comment
Hide comment
@mattvb91

mattvb91 Jan 12, 2016

Contributor

@sergeyklay can you restart the tests for this? Looks like composer failed?

Contributor

mattvb91 commented Jan 12, 2016

@sergeyklay can you restart the tests for this? Looks like composer failed?

@Jurigag

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Jan 12, 2016

Member

You can restart it yourself by close and open PR.

Also squash commits.

Also tests failed beacause of:

PHP Parse error:  syntax error, unexpected 'class' (T_CLASS), expecting identifier (T_STRING) or variable (T_VARIABLE) or '{' or '$' in /home/travis/build/phalcon/cphalcon/unit-tests/controllers/Test9Controller.php on line 9
Parse error: syntax error, unexpected 'class' (T_CLASS), expecting identifier (T_STRING) or variable (T_VARIABLE) or '{' or '$' in /home/travis/build/phalcon/cphalcon/unit-tests/controllers/Test9Controller.php on line 9
Member

Jurigag commented Jan 12, 2016

You can restart it yourself by close and open PR.

Also squash commits.

Also tests failed beacause of:

PHP Parse error:  syntax error, unexpected 'class' (T_CLASS), expecting identifier (T_STRING) or variable (T_VARIABLE) or '{' or '$' in /home/travis/build/phalcon/cphalcon/unit-tests/controllers/Test9Controller.php on line 9
Parse error: syntax error, unexpected 'class' (T_CLASS), expecting identifier (T_STRING) or variable (T_VARIABLE) or '{' or '$' in /home/travis/build/phalcon/cphalcon/unit-tests/controllers/Test9Controller.php on line 9

@mattvb91 mattvb91 closed this Jan 12, 2016

@mattvb91 mattvb91 reopened this Jan 12, 2016

@sergeyklay

View changes

Show outdated Hide outdated unit-tests/controllers/Test9Controller.php
@mattvb91

This comment has been minimized.

Show comment
Hide comment
@mattvb91

mattvb91 Jan 13, 2016

Contributor

Thanks have fixed but why would that cause tests to fail on 5.5 and 5.6?

Will squash it all once I can get travis to pass

Contributor

mattvb91 commented Jan 13, 2016

Thanks have fixed but why would that cause tests to fail on 5.5 and 5.6?

Will squash it all once I can get travis to pass

@sergeyklay

This comment has been minimized.

Show comment
Hide comment
@sergeyklay

sergeyklay Jan 13, 2016

Member

Did you tested/compelled locally?

Member

sergeyklay commented Jan 13, 2016

Did you tested/compelled locally?

@Jurigag

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Jan 13, 2016

Member

I have no idea whats going on with your travis build. You have segmntation fault and some tests are incomplete/skipped where you didnt even edit anything.

Member

Jurigag commented Jan 13, 2016

I have no idea whats going on with your travis build. You have segmntation fault and some tests are incomplete/skipped where you didnt even edit anything.

@mattvb91

This comment has been minimized.

Show comment
Hide comment
@mattvb91

mattvb91 Jan 14, 2016

Contributor

@sergeyklay yes I always compile locally and run tests here before pushing. Although I only have 5.6 available to test on at the moment I will look into getting 5.5 and 5.4 too.

I need to spend a bit more time looking into travis as im not sure what im looking for in the output.

Contributor

mattvb91 commented Jan 14, 2016

@sergeyklay yes I always compile locally and run tests here before pushing. Although I only have 5.6 available to test on at the moment I will look into getting 5.5 and 5.4 too.

I need to spend a bit more time looking into travis as im not sure what im looking for in the output.

@Jurigag

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Jan 14, 2016

Member

I got exactly the same errors here: #11306

Member

Jurigag commented Jan 14, 2016

I got exactly the same errors here: #11306

@mattvb91

This comment has been minimized.

Show comment
Hide comment
@mattvb91

mattvb91 Jan 14, 2016

Contributor

Yea looks like for the 5.6 log its lines 196 and 227 that are the only ones that have "install failed". I wonder if travis sees that as the reason for returning a 1 at the end.

Contributor

mattvb91 commented Jan 14, 2016

Yea looks like for the 5.6 log its lines 196 and 227 that are the only ones that have "install failed". I wonder if travis sees that as the reason for returning a 1 at the end.

@Jurigag

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Jan 14, 2016

Member

Not only 5.6, 5.5 and 5.4 have problems as well, i dont know why.

Member

Jurigag commented Jan 14, 2016

Not only 5.6, 5.5 and 5.4 have problems as well, i dont know why.

@Jurigag

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Jan 15, 2016

Member

@sergeyklay
Checked on locally machine, have segmentation fault as well. In syslog:
kernel: [3919505.262140] phpunit[17151]: segfault at 14 ip 00007ff63a5dbf18 sp 00007ffecf767930 error 6 in phalcon.so[7ff63a2bd000+38b000]
I can send you a crash file if needed.

@andresgutierrez
After running it in gdb:
Program received signal SIGSEGV, Segmentation fault.
0x00007fffea248f18 in phvolt_parse_view () from /usr/lib/php5/20121212/phalcon.so

Here you got output after writeing bt:
http://pastebin.com/8N3u8RcF

Not sure if its even helpful, cuz im not sure i have php with --enable-debug

(gdb) info symbol 0x00007fffea248f18
phvolt_parse_view + 24 in section .text of /usr/lib/php5/20121212/phalcon.so

EDIT:
Fixed seg fault in #11306

Member

Jurigag commented Jan 15, 2016

@sergeyklay
Checked on locally machine, have segmentation fault as well. In syslog:
kernel: [3919505.262140] phpunit[17151]: segfault at 14 ip 00007ff63a5dbf18 sp 00007ffecf767930 error 6 in phalcon.so[7ff63a2bd000+38b000]
I can send you a crash file if needed.

@andresgutierrez
After running it in gdb:
Program received signal SIGSEGV, Segmentation fault.
0x00007fffea248f18 in phvolt_parse_view () from /usr/lib/php5/20121212/phalcon.so

Here you got output after writeing bt:
http://pastebin.com/8N3u8RcF

Not sure if its even helpful, cuz im not sure i have php with --enable-debug

(gdb) info symbol 0x00007fffea248f18
phvolt_parse_view + 24 in section .text of /usr/lib/php5/20121212/phalcon.so

EDIT:
Fixed seg fault in #11306

@sergeyklay

This comment has been minimized.

Show comment
Hide comment
@sergeyklay

sergeyklay Jan 16, 2016

Member

Looks good to me. @andresgutierrez ?

Member

sergeyklay commented Jan 16, 2016

Looks good to me. @andresgutierrez ?

@andresgutierrez

This comment has been minimized.

Show comment
Hide comment
@andresgutierrez

andresgutierrez Jan 16, 2016

Member

Make a ReflectionMethod on every request could be slower. Can we make this optional?

Member

andresgutierrez commented Jan 16, 2016

Make a ReflectionMethod on every request could be slower. Can we make this optional?

@mattvb91

This comment has been minimized.

Show comment
Hide comment
@mattvb91

mattvb91 Jan 16, 2016

Contributor

Good point! What about a new method/parameter on dispatcher to enable/disable it?

$dispatcher->setModelBinding(true);

So then I can do a

if this->_modelBinding {
 //Code here
}
Contributor

mattvb91 commented Jan 16, 2016

Good point! What about a new method/parameter on dispatcher to enable/disable it?

$dispatcher->setModelBinding(true);

So then I can do a

if this->_modelBinding {
 //Code here
}
@mattvb91

This comment has been minimized.

Show comment
Hide comment
@mattvb91

mattvb91 Jan 16, 2016

Contributor

@andresgutierrez see a51aeac for making it optional.

Contributor

mattvb91 commented Jan 16, 2016

@andresgutierrez see a51aeac for making it optional.

@SidRoberts

This comment has been minimized.

Show comment
Hide comment
@SidRoberts

SidRoberts Jan 17, 2016

Member

I think this should be setModelBinding() (and this->_modelBinding) as setBindModel() might be confusing (it sounds like it accepts a Model as a parameter).

Member

SidRoberts commented on phalcon/dispatcher.zep in a51aeac Jan 17, 2016

I think this should be setModelBinding() (and this->_modelBinding) as setBindModel() might be confusing (it sounds like it accepts a Model as a parameter).

@SidRoberts

This comment has been minimized.

Show comment
Hide comment
@SidRoberts

SidRoberts Jan 17, 2016

Member

With regards to "Something for a next release would be to allow modifying the table column that it performs the select on instead of ID", you could get an array of the dispatcher parameters and whitelist them against the columns in the model with http://php.net/manual/en/function.array-diff.php. You could then do a findFirst and bind those parameters. Once I get home, I'd be happy to provide a proof-of-concept if it's not clear what I mean.

Member

SidRoberts commented Jan 17, 2016

With regards to "Something for a next release would be to allow modifying the table column that it performs the select on instead of ID", you could get an array of the dispatcher parameters and whitelist them against the columns in the model with http://php.net/manual/en/function.array-diff.php. You could then do a findFirst and bind those parameters. Once I get home, I'd be happy to provide a proof-of-concept if it's not clear what I mean.

@mattvb91

This comment has been minimized.

Show comment
Hide comment
@mattvb91

mattvb91 Jan 17, 2016

Contributor

Thanks for the feedback @SidRoberts I have updated to change the naming as you suggested, I agree it makes it alot clearer.

👍 for proof of concept, the thing that I am not sure about is how to decide which param is the correct one to choose if multiple get passed in that match. So just adds alot of complexity from the get go. I would like to have that functionality too though, maybe even a new model interface that if implemented gives you access to intercept the parameter/input and transform it before continuing?

Contributor

mattvb91 commented Jan 17, 2016

Thanks for the feedback @SidRoberts I have updated to change the naming as you suggested, I agree it makes it alot clearer.

👍 for proof of concept, the thing that I am not sure about is how to decide which param is the correct one to choose if multiple get passed in that match. So just adds alot of complexity from the get go. I would like to have that functionality too though, maybe even a new model interface that if implemented gives you access to intercept the parameter/input and transform it before continuing?

@Jurigag

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Jan 17, 2016

Member

Shouldnt we get getColumnMap for certain model from models metadata and then check if parameter exists as key in this array ?

Member

Jurigag commented Jan 17, 2016

Shouldnt we get getColumnMap for certain model from models metadata and then check if parameter exists as key in this array ?

@SidRoberts

This comment has been minimized.

Show comment
Hide comment
@SidRoberts

SidRoberts Jan 17, 2016

Member

@Jurigag yes, exactly that.

@mattvb91 I'd argue that if model binding is enabled and the action is expecting a model, then it is up to the developer to choose parameter names that do not interfere with that.

Member

SidRoberts commented Jan 17, 2016

@Jurigag yes, exactly that.

@mattvb91 I'd argue that if model binding is enabled and the action is expecting a model, then it is up to the developer to choose parameter names that do not interfere with that.

@@ -305,6 +307,16 @@ abstract class Dispatcher implements DispatcherInterface, InjectionAwareInterfac
}
/**
* Enable/Disable model binding during dispatch
*
* @param boolean value

This comment has been minimized.

@sergeyklay

sergeyklay Jan 17, 2016

Member

@param boolean value not needed here. It will be generated automatically

@sergeyklay

sergeyklay Jan 17, 2016

Member

@param boolean value not needed here. It will be generated automatically

@SidRoberts

This comment has been minimized.

Show comment
Hide comment
@SidRoberts

SidRoberts Jan 18, 2016

Member

OK, so I've been thinking about this PR and #11308 all night long. 😛 I'm not thinking very clearly so consider this some crazy guy's opinion until somebody else agrees with me. 😉

I personally don't think we should be interacting with dispatcher parameters via the action method's parameters (public function viewAction($param1, $param2)) - $this->dispatcher->getParam() should be used instead. If we didn't have to worry about compatibility, I'd be happy to disable the method parameters entirely.

I think getting model instances automatically from a route is a fantastic idea and one that Phalcon should have. However, I'm averse to changing the format of method parameters (which I believe to be already flawed) to achieve it. I'd also like to avoid using Reflection if at all possible.

I've come up with a slightly different take on this idea in the form of a new DI service:

<?php

namespace Sid;

class BoundModels extends \Phalcon\Mvc\User\Plugin
{
    /**
     * @param string     $className
     * @param array|null $acceptableAttributes
     *
     * @return \Phalcon\Mvc\ModelInterface|false
     */
    public function get($className, $acceptableAttributes = null)
    {
        if (!$acceptableAttributes) {
            $acceptableAttributes = $this->getDefaultAcceptableAttributes($className);
        }

        $conditions = [];
        $bind       = [];

        foreach ($acceptableAttributes as $attribute) {
            $conditions[]     = $attribute . " = :" . $attribute . ":";
            $bind[$attribute] = $this->dispatcher->getParam($attribute);
        }

        $conditions = implode(" AND ", $conditions);

        $boundModel = call_user_func_array(
            [$className, "findFirst"],
            [
                [
                    "conditions" => $conditions,
                    "bind"       => $bind
                ]
            ]
        );

        return $boundModel;
    }

    /**
     * @param string $className
     *
     * @return array
     */
    protected function getDefaultAcceptableAttributes($className)
    {
        $model = new $className();

        $dispatcherParams = array_keys($this->dispatcher->getParams());
        $modelAttributes  = $this->modelsMetadata->getAttributes($model);

        $acceptableAttributes = array_intersect($dispatcherParams, $modelAttributes);

        return $acceptableAttributes;
    }
}
$di->set(
    "boundModels",
    function () {
        $boundModels = new \Sid\BoundModels();

        return $boundModels;
    }
);
<?php

namespace Sid\Models;

class Posts extends \Phalcon\Mvc\Model
{
    public $categorySlug;
    public $postSlug;

    // ...
}
<?php

namespace Sid\Controllers;

/**
 * @RoutePrefix("/post")
 */
class PostController extends \Phalcon\Mvc\Controller
{
    /**
     * @Route("/{categorySlug:[A-Za-z0-9\-]+}/{postSlug:[A-Za-z0-9\-]+}")
     */
    public function postAction()
    {
        // Infer attributes
        $post = $this->boundModels->get("Sid\\Models\\Posts");

        // Force only categorySlug to be used to find the model
        $post = $this->boundModels->get("Sid\\Models\\Posts", ["categorySlug"]);

        // ...
    }
}
Member

SidRoberts commented Jan 18, 2016

OK, so I've been thinking about this PR and #11308 all night long. 😛 I'm not thinking very clearly so consider this some crazy guy's opinion until somebody else agrees with me. 😉

I personally don't think we should be interacting with dispatcher parameters via the action method's parameters (public function viewAction($param1, $param2)) - $this->dispatcher->getParam() should be used instead. If we didn't have to worry about compatibility, I'd be happy to disable the method parameters entirely.

I think getting model instances automatically from a route is a fantastic idea and one that Phalcon should have. However, I'm averse to changing the format of method parameters (which I believe to be already flawed) to achieve it. I'd also like to avoid using Reflection if at all possible.

I've come up with a slightly different take on this idea in the form of a new DI service:

<?php

namespace Sid;

class BoundModels extends \Phalcon\Mvc\User\Plugin
{
    /**
     * @param string     $className
     * @param array|null $acceptableAttributes
     *
     * @return \Phalcon\Mvc\ModelInterface|false
     */
    public function get($className, $acceptableAttributes = null)
    {
        if (!$acceptableAttributes) {
            $acceptableAttributes = $this->getDefaultAcceptableAttributes($className);
        }

        $conditions = [];
        $bind       = [];

        foreach ($acceptableAttributes as $attribute) {
            $conditions[]     = $attribute . " = :" . $attribute . ":";
            $bind[$attribute] = $this->dispatcher->getParam($attribute);
        }

        $conditions = implode(" AND ", $conditions);

        $boundModel = call_user_func_array(
            [$className, "findFirst"],
            [
                [
                    "conditions" => $conditions,
                    "bind"       => $bind
                ]
            ]
        );

        return $boundModel;
    }

    /**
     * @param string $className
     *
     * @return array
     */
    protected function getDefaultAcceptableAttributes($className)
    {
        $model = new $className();

        $dispatcherParams = array_keys($this->dispatcher->getParams());
        $modelAttributes  = $this->modelsMetadata->getAttributes($model);

        $acceptableAttributes = array_intersect($dispatcherParams, $modelAttributes);

        return $acceptableAttributes;
    }
}
$di->set(
    "boundModels",
    function () {
        $boundModels = new \Sid\BoundModels();

        return $boundModels;
    }
);
<?php

namespace Sid\Models;

class Posts extends \Phalcon\Mvc\Model
{
    public $categorySlug;
    public $postSlug;

    // ...
}
<?php

namespace Sid\Controllers;

/**
 * @RoutePrefix("/post")
 */
class PostController extends \Phalcon\Mvc\Controller
{
    /**
     * @Route("/{categorySlug:[A-Za-z0-9\-]+}/{postSlug:[A-Za-z0-9\-]+}")
     */
    public function postAction()
    {
        // Infer attributes
        $post = $this->boundModels->get("Sid\\Models\\Posts");

        // Force only categorySlug to be used to find the model
        $post = $this->boundModels->get("Sid\\Models\\Posts", ["categorySlug"]);

        // ...
    }
}
@mattvb91

This comment has been minimized.

Show comment
Hide comment
@mattvb91

mattvb91 Jan 18, 2016

Contributor

I do like the idea of being able to hook into it separately outside of the dispatcher. However personally if the end result is still having to do:

 $post = $this->boundModels->get("Sid\\Models\\Posts");

instead of

function postAction(Posts $post)

Then I do think its nearly the same result as just simply doing a findFirst() ?
I wonder if we could create a new event "beforeDispatch" or something and then hook something in like that? Personally I was looking for the Laravel style loading passed into the function instead of then having to fetch it again inside.

Contributor

mattvb91 commented Jan 18, 2016

I do like the idea of being able to hook into it separately outside of the dispatcher. However personally if the end result is still having to do:

 $post = $this->boundModels->get("Sid\\Models\\Posts");

instead of

function postAction(Posts $post)

Then I do think its nearly the same result as just simply doing a findFirst() ?
I wonder if we could create a new event "beforeDispatch" or something and then hook something in like that? Personally I was looking for the Laravel style loading passed into the function instead of then having to fetch it again inside.

@mattvb91

This comment has been minimized.

Show comment
Hide comment
@mattvb91

mattvb91 Jan 20, 2016

Contributor

Anyone else have an opinion regarding the above?

Contributor

mattvb91 commented Jan 20, 2016

Anyone else have an opinion regarding the above?

@Jurigag

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Jan 20, 2016

Member

I think di service is just some thing you can do always. To be honest its even mean thay you still have to do everything by yourself, i like original idea more, but its good idea too that it wouldnt necessary create reflectionmethod each time on each request. Maybe go back to interface - and make it necessary to implement it if we want this behaviour ? I guess its faster to check that object is instanceof something instead of creating reflection and check type of parameters etc.

Member

Jurigag commented Jan 20, 2016

I think di service is just some thing you can do always. To be honest its even mean thay you still have to do everything by yourself, i like original idea more, but its good idea too that it wouldnt necessary create reflectionmethod each time on each request. Maybe go back to interface - and make it necessary to implement it if we want this behaviour ? I guess its faster to check that object is instanceof something instead of creating reflection and check type of parameters etc.

@mattvb91

This comment has been minimized.

Show comment
Hide comment
@mattvb91

mattvb91 Jan 20, 2016

Contributor

Agreed it would be nice to not have to create a reflection method each time. Personally I would be happy with it if it was necessary to implement the interface for it to work in order to get rid of the reflectionmethod. Although it is a pretty slick looking controller when you can just typehint the model without having to implement the getModelName method.

Another thing to think about: Like I said in my initial RFC issue personally I only use 1 controller for 1 model so the above will work fine for me. However what about someone who uses multiple models within a controller and would like to typehint different models on different functions? This wouldnt work without the reflection method.

If people are worried about that reflectionmethod on every request, what about taking it even one step further and instead of on the dispatcher having the setModelBinding(true) we instead have a method per controller? That way ONLY if the controller specifically returns true for the setModelBinding method only then will it use the reflectionMethod. Any other controller that doesnt setModelBinding(true) will just skip over it?

Contributor

mattvb91 commented Jan 20, 2016

Agreed it would be nice to not have to create a reflection method each time. Personally I would be happy with it if it was necessary to implement the interface for it to work in order to get rid of the reflectionmethod. Although it is a pretty slick looking controller when you can just typehint the model without having to implement the getModelName method.

Another thing to think about: Like I said in my initial RFC issue personally I only use 1 controller for 1 model so the above will work fine for me. However what about someone who uses multiple models within a controller and would like to typehint different models on different functions? This wouldnt work without the reflection method.

If people are worried about that reflectionmethod on every request, what about taking it even one step further and instead of on the dispatcher having the setModelBinding(true) we instead have a method per controller? That way ONLY if the controller specifically returns true for the setModelBinding method only then will it use the reflectionMethod. Any other controller that doesnt setModelBinding(true) will just skip over it?

@Green-Cat

This comment has been minimized.

Show comment
Hide comment
@Green-Cat

Green-Cat Jan 21, 2016

Contributor

How significant is the performance impact from the reflection method anyway?

Contributor

Green-Cat commented Jan 21, 2016

How significant is the performance impact from the reflection method anyway?

@mattvb91

This comment has been minimized.

Show comment
Hide comment
@mattvb91

mattvb91 Jan 21, 2016

Contributor

I wouldn't even know how to test that through zephir? If it was being used in a loop of some kind I would be worried about some performance hit but it would only be used once during the request to get to the controller.

Contributor

mattvb91 commented Jan 21, 2016

I wouldn't even know how to test that through zephir? If it was being used in a loop of some kind I would be worried about some performance hit but it would only be used once during the request to get to the controller.

@mattvb91

This comment has been minimized.

Show comment
Hide comment
@mattvb91

mattvb91 Jan 28, 2016

Contributor

@andresgutierrez any feedback since making it optional?

Contributor

mattvb91 commented Jan 28, 2016

@andresgutierrez any feedback since making it optional?

@SidRoberts

This comment has been minimized.

Show comment
Hide comment
@SidRoberts

SidRoberts Jan 28, 2016

Member

FWIW, I've also turned my implementation (with a few tweaks) into a Composer package: https://packagist.org/packages/sidroberts/phalcon-boundmodels

Member

SidRoberts commented Jan 28, 2016

FWIW, I've also turned my implementation (with a few tweaks) into a Composer package: https://packagist.org/packages/sidroberts/phalcon-boundmodels

andresgutierrez added a commit that referenced this pull request Jan 28, 2016

Merge pull request #11288 from mattvb91/2.1.x
Load model in dispatcher if type hinted in controller action

@andresgutierrez andresgutierrez merged commit baf4748 into phalcon:2.1.x Jan 28, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@andresgutierrez

This comment has been minimized.

Show comment
Hide comment
@andresgutierrez
Member

andresgutierrez commented Jan 28, 2016

Thanks

@sergeyklay

This comment has been minimized.

Show comment
Hide comment
@sergeyklay

sergeyklay Jan 28, 2016

Member

@mattvb91 The docs should probably be updated too. Can you please create PR at least for English?

Member

sergeyklay commented Jan 28, 2016

@mattvb91 The docs should probably be updated too. Can you please create PR at least for English?

@mattvb91

This comment has been minimized.

Show comment
Hide comment
@mattvb91

mattvb91 Jan 28, 2016

Contributor

@sergeyklay yes was planning on doing that! Will have it in this weekend!

Contributor

mattvb91 commented Jan 28, 2016

@sergeyklay yes was planning on doing that! Will have it in this weekend!

@sergeyklay

This comment has been minimized.

Show comment
Hide comment
@sergeyklay

sergeyklay Jan 28, 2016

Member

@mattvb91 You have to note, this feature is available from 2.1.x on

Member

sergeyklay commented Jan 28, 2016

@mattvb91 You have to note, this feature is available from 2.1.x on

@daison12006013

This comment has been minimized.

Show comment
Hide comment
@daison12006013

daison12006013 Jan 28, 2016

Is this more likely a method injection? this is cool similar with other frameworks 👍

daison12006013 commented Jan 28, 2016

Is this more likely a method injection? this is cool similar with other frameworks 👍

@sergeyklay

This comment has been minimized.

Show comment
Hide comment
@sergeyklay

sergeyklay Jan 28, 2016

Member

@mattvb91 @daison12006013 We understand to which framework it is most similar 😉

Member

sergeyklay commented Jan 28, 2016

@mattvb91 @daison12006013 We understand to which framework it is most similar 😉

@Jurigag

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Mar 22, 2016

Member

@mattvb91

Could you possibly add new event - afterBinding but before executing action to dispatcher and method like getBindedModel() ? I'm currently implementing some firewall to phalcon, currently using annotations, but i want to add option to using internally phalcon's ACL, and this binded model will be passed to user defined function in acl(new ability in 2.1.x).

If you don't have time i will implement it myself.

Member

Jurigag commented Mar 22, 2016

@mattvb91

Could you possibly add new event - afterBinding but before executing action to dispatcher and method like getBindedModel() ? I'm currently implementing some firewall to phalcon, currently using annotations, but i want to add option to using internally phalcon's ACL, and this binded model will be passed to user defined function in acl(new ability in 2.1.x).

If you don't have time i will implement it myself.

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