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.

Sign up for GitHub

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

Jump to bottom

Fix Bug 81160 #7173

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix Bug 81160 #7173

wants to merge 2 commits into from

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Jun 18, 2021 •
edited by nikic
Loading

@Girgias Girgias added the Bug label Jun 18, 2021
@Girgias Girgias requested a review from nikic July 9, 2021 14:48
nikic
nikic reviewed Jul 15, 2021
View reviewed changes
Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expect this needs JIT changes, e.g. zend_jit_fetch_dim_str_is_helper?

Zend/zend_execute.c Outdated Show resolved Hide resolved
@Girgias
Copy link
Member Author

Girgias commented Jul 17, 2021

I expect this needs JIT changes, e.g. zend_jit_fetch_dim_str_is_helper?

I don't think it does, the JIT doesn't support empty(), and looking at the code it seems it bails out it the container is not an array/string/object.

@Girgias Girgias requested a review from nikic September 7, 2021 12:26
@Girgias
Copy link
Member Author

Girgias commented Sep 7, 2021

Finally got back to this, and I think I've now fixed the JIT issues.

For future reference, to see the JIT failures php-src needs to be compiled with CFLAGS="-DPROFITABILITY_CHECKS=0" and the test runner needs to be run with the following arguments: -d opcache.jit=function -d opcache.enable=1 -d opcache.enable_cli=1 -d opcache.file_update_protection=0 -d opcache.jit_buffer_size=1M

@Girgias Girgias changed the base branch from master to PHP-8.1 September 13, 2021 12:51
@Girgias Girgias force-pushed the fix-81160 branch 2 times, most recently from 24337d6 to 26d75b6 Compare April 1, 2022 15:31
@Girgias Girgias changed the base branch from PHP-8.1 to master April 1, 2022 15:32
@Girgias Girgias force-pushed the fix-81160 branch 2 times, most recently from 1e63a8e to 7e04ded Compare April 5, 2022 12:06
@Girgias Girgias requested review from arnaud-lb, iluuu1994 and kocsismate and removed request for nikic June 9, 2022 12:56
iluuu1994
iluuu1994 reviewed Jun 9, 2022
View reviewed changes
Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was the BC break discussed before? I think this warrants an e-mail to the internals list.

Zend/Optimizer/zend_inference.c Outdated Show resolved Hide resolved
@Girgias Girgias mentioned this pull request Jan 27, 2023
Closed
@kocsismate
Copy link
Member

Just my 2 cents: I'm not overly enthusiastic about this change: I find it quite useful to be able to use isset and empty on any type to check in the same time whether the input is an array and the given offset exists.

@Girgias
Copy link
Member Author

Girgias commented Feb 13, 2023

Just my 2 cents: I'm not overly enthusiastic about this change: I find it quite useful to be able to use isset and empty on any type to check in the same time whether the input is an array and the given offset exists.

The thing is that arrays already throw TypeErrors, I'm planning to go write an RFC for this but I haven't had the time yet.

staabm
staabm reviewed Feb 13, 2023
View reviewed changes
UPGRADING Outdated Show resolved Hide resolved
@Girgias Girgias mentioned this pull request Mar 23, 2023
Closed
@Girgias Girgias requested a review from dstogov as a code owner October 7, 2023 13:51
Girgias and others added 2 commits November 17, 2023 01:46
@Girgias @iluuu1994
Fix Bug #81160: isset/empty doesn't throw a TypeError on invalid stri…
f412bc6 
…ng offset

Co-authored-by: Ilija Tovilo <ilija.tovilo@me.com>
@Girgias
Upgrading
19f643d
@Girgias Girgias mentioned this pull request Nov 20, 2023
Merged
@dstogov
Copy link
Member

dstogov commented Nov 20, 2023

I'm not sure if this was a bug.
isset() might silently returns false by design.
Anyway, I think, this change may break the existing apps and requires RFC.

@Girgias
Copy link
Member Author

Girgias commented Nov 20, 2023

I'm not sure if this was a bug. isset() might silently returns false by design. Anyway, I think, this change may break the existing apps and requires RFC.

Yes, I am planning on doing an RFC :)

staabm
staabm reviewed Nov 20, 2023
View reviewed changes
UPGRADING
Comment on lines +26 to +28
$a = 'string';
var_dump(isset($a['b']));
var_dump(empty($a['b']));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw: PHPStan also warns in this constellation:

https://phpstan.org/r/3b26cd4f-bdd1-470a-9b8f-1f42eb1386ee

@Girgias Girgias mentioned this pull request Dec 1, 2023
Closed
@Girgias Girgias mentioned this pull request Jan 25, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iluuu1994 iluuu1994 iluuu1994 left review comments

@nikic nikic nikic left review comments

@staabm staabm staabm left review comments

@dstogov dstogov Awaiting requested review from dstogov dstogov is a code owner

Assignees
No one assigned
Labels
Bug Category: Engine Category: Optimizer Extension: opcache
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Girgias @kocsismate @dstogov @staabm @nikic @iluuu1994