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

Bugfix #10912 (Micro Middleware (before) does not stop operation) #10931

Merged
merged 1 commit into from Apr 26, 2017

Conversation

Projects
None yet
6 participants
@pauliuspetronis
Contributor

pauliuspetronis commented Sep 11, 2015

In this pull request is one important change.

before this change:

  1. if event (before) is closure and return false - cancels the route execution
  2. if event (before) is MiddlewareInterface object and return false/true/... - not cancels route execution

In this change:

  1. if event (before) is closure and return false/true/... - not cancels route execution
  2. if event (before) is MiddlewareInterface and return false/true/... - not cancels route execution

If you need to cancel execution on event (before) - in both cases must be called $microApplication->stop();

For example:

<?php
$app = new Phalcon\Mvc\Micro();
$app->before(function () use ($app) {
    if ($app['session']->get('auth') == false) {
        $app->stop();
        return false;
    }
    return true;
});
$app->handle();

or

<?php
class Auth implements Phalcon\Mvc\Micro\MiddlewareInterface
{
    public function call(Phalcon\Mvc\Micro $app)
    {
         if ($app['session']->get('auth') == false) {
            $app->stop();
            return false;
        }
        return true;
    }
}
$app = new Phalcon\Mvc\Micro();
$app->before(new Access());
$app->handle();
@andresgutierrez

This comment has been minimized.

Show comment
Hide comment
@andresgutierrez

andresgutierrez Sep 11, 2015

Member

Just to make sure, return false does not stop the events?

Member

andresgutierrez commented Sep 11, 2015

Just to make sure, return false does not stop the events?

@pauliuspetronis

This comment has been minimized.

Show comment
Hide comment
@pauliuspetronis

pauliuspetronis Sep 12, 2015

Contributor

Yes.
In before event - will cancel process only if you call stop() function.
In after and finish events - will never cancel process, but if event call stop() then skip next same type events (after, or finish).

Contributor

pauliuspetronis commented Sep 12, 2015

Yes.
In before event - will cancel process only if you call stop() function.
In after and finish events - will never cancel process, but if event call stop() then skip next same type events (after, or finish).

@sszdh

This comment has been minimized.

Show comment
Hide comment
@sszdh

sszdh commented Sep 12, 2015

👍

@sergeyklay

This comment has been minimized.

Show comment
Hide comment
@sergeyklay

sergeyklay Nov 6, 2015

Member

👍

Member

sergeyklay commented Nov 6, 2015

👍

@sjinks sjinks closed this Apr 26, 2017

@sjinks sjinks reopened this Apr 26, 2017

@sergeyklay

This comment has been minimized.

Show comment
Hide comment
@sergeyklay

sergeyklay Apr 26, 2017

Member

@pauliuspetronis Could you please rebase onto 3.2.x branch

Member

sergeyklay commented Apr 26, 2017

@pauliuspetronis Could you please rebase onto 3.2.x branch

@sergeyklay sergeyklay self-assigned this Apr 26, 2017

@pauliuspetronis pauliuspetronis changed the base branch from 2.1.x to 3.2.x Apr 26, 2017

@pauliuspetronis

This comment has been minimized.

Show comment
Hide comment
@pauliuspetronis
Contributor

pauliuspetronis commented Apr 26, 2017

@sergeyklay - done

@sergeyklay

Looks good to me

@sergeyklay

This comment has been minimized.

Show comment
Hide comment
@sergeyklay

sergeyklay Apr 26, 2017

Member

@pauliuspetronis Could you please update CHANGELOG.md

Member

sergeyklay commented Apr 26, 2017

@pauliuspetronis Could you please update CHANGELOG.md

@sergeyklay sergeyklay added this to the 3.2.0 milestone Apr 26, 2017

@pauliuspetronis

This comment has been minimized.

Show comment
Hide comment
@pauliuspetronis

pauliuspetronis Apr 26, 2017

Contributor

@sergeyklay - updated. But I think the same problem is with afterBindingHandlers. Or I'm wrong?

Contributor

pauliuspetronis commented Apr 26, 2017

@sergeyklay - updated. But I think the same problem is with afterBindingHandlers. Or I'm wrong?

@sergeyklay sergeyklay merged commit 6e60e8b into phalcon:3.2.x Apr 26, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sergeyklay

This comment has been minimized.

Show comment
Hide comment
@sergeyklay

sergeyklay Apr 26, 2017

Member

@pauliuspetronis Looks like yes. Anyway it should be in separated PR. Thank you for contributing !

Member

sergeyklay commented Apr 26, 2017

@pauliuspetronis Looks like yes. Anyway it should be in separated PR. Thank you for contributing !

@pauliuspetronis pauliuspetronis deleted the pauliuspetronis:BugFix10912in2.1.x branch Apr 26, 2017

@pauliuspetronis pauliuspetronis referenced this pull request Apr 26, 2017

Merged

Bugfix afterBinding handling #12821

3 of 3 tasks complete
@necipallef

This comment has been minimized.

Show comment
Hide comment
@necipallef

necipallef Sep 21, 2017

First of all, I would like to say thanks for this awesome framework and awesome work. Really appreciate it. We love using PHP and Phalcon in our projects, we've already written several separate libraries to make the best use of Phalcon (here is an example).

However, by changing a crucial functionality like this (calling return false does not stop the execution on before) broke most of our applications that are live today, causing huge security issues. It is unpleasant to be aware of this after it is been a while since this change was released.

Maybe I have to check here more often, but we are already dependent on lots of other libraries too, and checking them all is not always possible. Please be more careful not to break people's code who loves using Phalcon in the future.

necipallef commented Sep 21, 2017

First of all, I would like to say thanks for this awesome framework and awesome work. Really appreciate it. We love using PHP and Phalcon in our projects, we've already written several separate libraries to make the best use of Phalcon (here is an example).

However, by changing a crucial functionality like this (calling return false does not stop the execution on before) broke most of our applications that are live today, causing huge security issues. It is unpleasant to be aware of this after it is been a while since this change was released.

Maybe I have to check here more often, but we are already dependent on lots of other libraries too, and checking them all is not always possible. Please be more careful not to break people's code who loves using Phalcon in the future.

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