Skip to content

Do not generate CONST_CS when registering constants #9439

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
Merged
merged 1 commit into from
Aug 28, 2022
Merged

Do not generate CONST_CS when registering constants #9439

merged 1 commit into from
Aug 28, 2022

Conversation

kocsismate
Copy link
Member

@kocsismate kocsismate commented Aug 27, 2022 •
edited
Loading

CONST_CS hasn't been in use for quite a while.

@kocsismate kocsismate requested a review from cmb69 August 27, 2022 18:10
TimWolla
TimWolla approved these changes Aug 27, 2022
View reviewed changes
Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

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

LGTM as per #9135 (review) 😃

@kocsismate kocsismate merged commit b4ec3e9 into php:master Aug 28, 2022
@kocsismate kocsismate deleted the remove-cost-cs branch August 28, 2022 06:27
@TysonAndre
Copy link
Contributor

TysonAndre commented Oct 7, 2022 •
edited
Loading

build/gen_stubs.php is also used by some PECL extensions or being encouraged to be used. Extension authors using gen_stub.php (or adopting it for the first time) might miss the effect this has when their extensions are built for php 7.4 and below - existing tests would pass, and end users would only notice if they attempted to use the wrong case for the constant.

There's also no way to force build/gen_stubs.php to emit CONST_CS (e.g. even for the legacy arginfo flags)

» php -a
Interactive shell

php > var_export(SOME_CONSTANT_FROM_STUBS);
283
php > var_export(some_constant_from_stubs);
Deprecated: Case-insensitive constants are deprecated. The correct casing for this constant is "SOME_CONSTANT_FROM_STUBS" in php shell code on line 1
283

Maybe there could be a .php_stubs_config.json file (or properties/env style file) and this could check all parent directories, if we want to apply certain settings that only work in php 8.3+ to all folders in php-src. Or just the existence of a .php_stubs_use_latest file, though a full config file would be better for customizations if we wanted them in the future

@kocsismate
Copy link
Member Author

Thank you for bringing the issue to my attention! I retained the original behavior for PHP 7.x in 69ef324.

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

@TimWolla TimWolla TimWolla approved these changes

@cmb69 cmb69 Awaiting requested review from cmb69

Assignees
No one assigned
Labels
Category: Build System Category: Engine Extension: calendar Extension: com_dotnet Extension: curl Extension: date Extension: dba Extension: dom Extension: enchant Extension: exif Extension: fileinfo Extension: filter Extension: ftp Extension: gd Extension: gmp Extension: hash Extension: iconv Extension: imap Extension: intl Extension: json Extension: ldap Extension: libxml Extension: mbstring Extension: mysqli Extension: oci8 Extension: odbc Extension: openssl Extension: pcntl Extension: pcre Extension: pgsql Extension: posix Extension: pspell Extension: random Extension: readline Extension: session Extension: snmp Extension: soap Extension: sockets Extension: sodium Extension: sqlite3 Extension: standard Extension: sysvmsg Extension: tidy Extension: tokenizer Extension: xml Extension: xsl Extension: zend_test Extension: zlib SAPI: phpdbg
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kocsismate @TysonAndre @TimWolla