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

[BUG]: Can't setParams to dispatcher beforeExecuteRoute #15603

Closed
crewsycrews opened this issue Aug 2, 2021 · 10 comments · Fixed by #15794 or #15795
Closed

[BUG]: Can't setParams to dispatcher beforeExecuteRoute #15603

crewsycrews opened this issue Aug 2, 2021 · 10 comments · Fixed by #15794 or #15795
Assignees
Labels
5.0 The issues we want to solve in the 5.0 release bug A bug report status: medium Medium

Comments

@crewsycrews
Copy link

crewsycrews commented Aug 2, 2021

Describe the bug

We want to setParams() of the dispatcher on beforeExecuteRoute event but it have no effect on the method.

Expected behavior

An ability to setParams() on beforeExecuteRoute event.

    /**
     * @param Event $event
     * @param Dispatcher $dispatcher
     * @throws ReflectionException
     */
    public function beforeExecuteRoute(Event $event, Dispatcher $dispatcher): void
    {
        $controllerName = $dispatcher->getControllerClass();
        $actionName = $dispatcher->getActiveMethod();
        try {
            $reflection = new ReflectionMethod($controllerName, $actionName);
        } catch (ReflectionException $e) {
            return;
        }

        $parameters = $this->instantiateParams($reflection, $dispatcher->getParams());
        // next line has no effect here
        $dispatcher->setParams($parameters);
    }

Details

  • Phalcon version: 4.0.5
  • PHP Version: 7.4.15
  • Operating System: Linux
  • Installation type: installing via package manager
  • Zephir version (if any): Version 0.12.17-6724dbf
  • Server: Nginx
@BeMySlaveDarlin
Copy link
Contributor

BeMySlaveDarlin commented Nov 7, 2021

beforeExecuteRoute expects first argument to be instance of Dispatcher, not Event

class ControllerBase extends Controller
{
    public function beforeExecuteRoute(Dispatcher $dispatcher): void
    {
        $dispatcher->setParams([1, 2, 3]);
    }
}

class IndexController extends ControllerBase
{
    public function indexAction()
    {
        var_dump($this->dispatcher->getParams());
        die();
    }
}

//array(3) { [0]=> int(1) [1]=> int(2) [2]=> int(3) }

@BeMySlaveDarlin BeMySlaveDarlin added not a bug Reported issue is not a bug bug A bug report and removed status: unverified Unverified bug A bug report labels Nov 7, 2021
@Jeckerson Jeckerson added wontfix The issue will not be fixed or implemented and removed bug A bug report labels Nov 7, 2021
@Jeckerson Jeckerson added this to Unverified - Wont Fix - Duplicates in Phalcon Roadmap Nov 7, 2021
Phalcon Roadmap automation moved this from Unverified - Wont Fix - Duplicates to Implemented Nov 7, 2021
@niden niden moved this from Implemented to Released in Phalcon Roadmap Nov 16, 2021
@crewsycrews
Copy link
Author

crewsycrews commented Nov 17, 2021

Sorry, I've not mentioned that beforeExecuteRoute declared in ServiceProvider, not in the controller. Can you check this one more time, please?

@kirill-voronov
Copy link

kirill-voronov commented Nov 17, 2021

This happens because in line 335 of Dispatcher params are copied into local method variable

let params = this->params;

and since then only that variable is used, so any further call of $dispatcher->setParams has no effect, including such call in beforeExecuteRoute callback (in controller or plugin), because setParams modifies this->params not local params variable.

@kirill-voronov
Copy link

@niden
Copy link
Sponsor Member

niden commented Nov 17, 2021

...
and since then only that variable is used, so any further call of $dispatcher->setParams has no effect, including such call in beforeExecuteRoute callback, because setParams modifies this->params not local params variable.

Reopening this - Thanks for pointing it out @kirill-voronov

@niden niden reopened this Nov 17, 2021
@niden niden added 5.0 The issues we want to solve in the 5.0 release bug A bug report status: medium Medium and removed not a bug Reported issue is not a bug wontfix The issue will not be fixed or implemented labels Nov 17, 2021
@niden niden moved this from Released to Working on it in Phalcon Roadmap Nov 17, 2021
BeMySlaveDarlin pushed a commit to BeMySlaveDarlin/cphalcon that referenced this issue Nov 17, 2021
BeMySlaveDarlin pushed a commit to BeMySlaveDarlin/cphalcon that referenced this issue Nov 17, 2021
@BeMySlaveDarlin BeMySlaveDarlin linked a pull request Nov 17, 2021 that will close this issue
5 tasks
@BeMySlaveDarlin
Copy link
Contributor

@niden Looks like events firing during dispatch() method run doesn't affects params attribute because it was assigned to local variable as @kirill-voronov mentioned.
Made minor fix in #15794

@BeMySlaveDarlin
Copy link
Contributor

BeMySlaveDarlin commented Nov 17, 2021

Case using in controller or dispatcher itself fires before dispatch loop and works fine, but when using events managed way - it fires during dispatch loop and doesnt affects params

BeMySlaveDarlin pushed a commit to BeMySlaveDarlin/cphalcon that referenced this issue Nov 17, 2021
@niden
Copy link
Sponsor Member

niden commented Nov 17, 2021

Resolved in #15794

@niden niden closed this as completed Nov 17, 2021
Phalcon Roadmap automation moved this from Working on it to Implemented Nov 17, 2021
@niden niden linked a pull request Nov 18, 2021 that will close this issue
5 tasks
@kirill-voronov
Copy link

Thanks y'all ❤️

@niden
Copy link
Sponsor Member

niden commented Nov 18, 2021

@kirill-voronov Thank you for finding this. The second PR has tests that confirm that this works as expected.

#15795

@niden niden moved this from Implemented to Released in Phalcon Roadmap Dec 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5.0 The issues we want to solve in the 5.0 release bug A bug report status: medium Medium
Projects
Archived in project
Phalcon Roadmap
  
Released
5 participants