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

Fix unit test for issue #12648 - Incorrect scope of view variables #13288

Closed
wants to merge 1 commit into
base: 3.3.x
from

Conversation

Projects
None yet
3 participants
@piit79

piit79 commented Jan 30, 2018

Hello!

  • Type: bug fix
  • Link to issue: #12648

In raising this pull request, I confirm the following:

  • I have read and understood the Contributing Guidelines?
  • I have checked that another pull request for this purpose does not exist.
  • I wrote some tests for this PR.

Small description of change:
The unit test for issue #12648 is incorrect and doesn't test the failure condition of the issue properly. The issue is marked as fixed but the problem is still present in 3.3.x.

Thanks
Petr Sedlacek

@piit79 piit79 force-pushed the piit79:issue12648 branch 5 times, most recently from 61ea094 to df8b3fb Jan 30, 2018

@piit79

This comment has been minimized.

piit79 commented Feb 8, 2018

@sergeyklay Just to be sure, could you please take a look at the unit test and confirm it's correct? It seems strange to me nobody has complained about #12648 which makes me think if we're doing something wrong :) We're using parametric partials which does not work with PHP 7 (the variables leak between partials). Thanks for your help on this!

@sergeyklay sergeyklay assigned sergeyklay and Jurigag and unassigned sergeyklay Feb 8, 2018

@sergeyklay sergeyklay requested a review from Jurigag Feb 8, 2018

@sergeyklay

This comment has been minimized.

Member

sergeyklay commented Feb 8, 2018

/ cc @Jurigag

@sergeyklay

This comment has been minimized.

Member

sergeyklay commented Apr 9, 2018

@piit79 As I can see it happens only in Zend Engine 3 (PHP 7).

Refs: phalcon/zephir#1621, #12176

@piit79

This comment has been minimized.

piit79 commented Apr 10, 2018

@sergeyklay that's right, we only started having the issue when we switched to PHP 7 recently (with the same code). Thanks for looking into this.

@sergeyklay sergeyklay assigned sergeyklay and unassigned Jurigag Apr 11, 2018

@sergeyklay sergeyklay removed the request for review from Jurigag Apr 11, 2018

@sergeyklay

This comment has been minimized.

Member

sergeyklay commented Apr 11, 2018

@piit79 I'll try to sort out with this

sergeyklay added a commit that referenced this pull request Nov 20, 2018

sergeyklay added a commit that referenced this pull request Nov 20, 2018

@sergeyklay

This comment has been minimized.

Member

sergeyklay commented Nov 20, 2018

I hope we managed to sort out with this issue finally here #13606. All necessary changes are already in the appropriate branch. We will try to release the next version in the coming days. Thank you for your patience, the report, and for helping us make Phalcon better.

@piit79

This comment has been minimized.

piit79 commented Nov 21, 2018

@sergeyklay That's great! Can you please point me to the branch? I'll do a quick test.

However, I see you haven't merged the fix for the unit test, can I ask why? It's still broken in master:

            try {
                echo $a_cool_var;
            } catch (\PHPUnit_Framework_Exception $e) {
                expect($e->getMessage())->contains("Undefined variable: a_cool_var");
            }

The problem is that if the variable $a_cool_var is actually set (which it should not), there will be no exception and the test will not catch this failure condition. That's why I added the fail() after the echo.

@sergeyklay

This comment has been minimized.

Member

sergeyklay commented Nov 21, 2018

Please see: #12648 (comment)

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