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

[Cache][Lock] Fix usages of error_get_last() #27232

Merged
merged 1 commit into from May 14, 2018

Conversation

@nicolas-grekas
Copy link
Member

commented May 11, 2018

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

When a userland error handler doesn't return false, error_get_last() is not updated, so we cannot see the real last error, but the previous one.

See https://3v4l.org/Smmt7

if (@!$redis->isConnected()) {
set_error_handler('var_dump', 0);
$isConnected = @$redis->isConnected();
restore_error_handler();

This comment has been minimized.

Copy link
@linaori

linaori May 11, 2018

Contributor

Symfony often has something similar to this. Would it make sense at add a small abstraction for this?

$isConnected = ErrorFree::handle(function () use ($redis) {
    return @$redis->isConnected();
});

Advantage is that user-land can benefit from this abstraction as well for libraries or application implementations that need to do something similar. Primarily referring to something similar to: https://github.com/symfony/symfony/pull/27232/files#diff-e88ad8ede40599bea0592b1113c4452cR744

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas May 11, 2018

Author Member

The issue is where would we put this? There is no global "utils" component, and I'm not sure we want one...

This comment has been minimized.

Copy link
@linaori

linaori May 11, 2018

Contributor

I would pick the same location (for now) as where the error handlers are located. If I'm not mistaken, this is the debug component. Downside, it becomes a centralized dependency...

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas May 11, 2018

Author Member

it becomes a centralized dependency

that's the issue, it's not justified imho, the added lines are just fine

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas May 11, 2018

Author Member

Note that symfony/abstractions might be a place for global and very general functions doing this, but I'm not sure at all right now, it should be discussed separately.

@fabpot

This comment has been minimized.

Copy link
Member

commented May 11, 2018

Why is it on 3.4 and not 2.7?

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:error-get-last branch from 97c7924 to ff4bb8f May 11, 2018
@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented May 11, 2018

See #27236 for 2.7 (HHVM might be red for the Lock component, I need to check this before merging here.)

@nicolas-grekas nicolas-grekas changed the title [Filesystem] Fix usages of error_get_last() [Cache][Lock] Fix usages of error_get_last() May 11, 2018
@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:error-get-last branch from ff4bb8f to ac4ceb8 May 11, 2018
@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:error-get-last branch from ac4ceb8 to 7904784 May 11, 2018
fabpot added a commit that referenced this pull request May 14, 2018
This PR was merged into the 2.7 branch.

Discussion
----------

[Filesystem] Fix usages of error_get_last()

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Same as #27232 for 2.7.
When a userland error handler doesn't return `false`, `error_get_last()` is not updated, so we cannot see the real last error, but the previous one.

See https://3v4l.org/Smmt7

Commits
-------

9d015c7 [Filesystem] Fix usages of error_get_last()
@fabpot
fabpot approved these changes May 14, 2018
@fabpot

This comment has been minimized.

Copy link
Member

commented May 14, 2018

Thank you @nicolas-grekas.

@fabpot fabpot merged commit 7904784 into symfony:3.4 May 14, 2018
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details
fabpot added a commit that referenced this pull request May 14, 2018
This PR was merged into the 3.4 branch.

Discussion
----------

[Cache][Lock] Fix usages of error_get_last()

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

When a userland error handler doesn't return `false`, `error_get_last()` is not updated, so we cannot see the real last error, but the previous one.

See https://3v4l.org/Smmt7

Commits
-------

7904784 [Cache][Lock] Fix usages of error_get_last()
@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:error-get-last branch May 16, 2018
This was referenced May 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.