-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Remove unnecessary usage of CONST_CS #9685
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
Conversation
#define CONST_CS 0 /* No longer used -- always case sensitive */
This seems to work correctly - the build/gen_stubs.php script also seems to omit CONST_CS in new arginfo stubs
Not related to this PR specifically, but speaking of which
- CONST_CS could document that it stopped being used in PHP 8.0
- A review comment on https://github.com/php/php-src/pull/9439/files - https://wiki.php.net/rfc/case_insensitive_constant_deprecation only took effect in php 8.0? But the build/gen_stubs.php script is also used by PECLs, many of which still also support PHP 7.4 or older (which still has security support) - Not even $fileInfo->generateLegacyArginfoForPhpVersionId for 70000 seems like it would emit code with CONST_CS
ext/com_dotnet/com_com.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should just inline that 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I changed this. We can also simplify function php_com_import_typelib with the removal of CONST_CS, but I'm now learning how PHP source code works and don't want to rush.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I don't think that we can simplify php_com_import_typelib, since the mode is checked there for CONST_PERSISTENT which is still supported.
Thank you! I'm in favor of dropping the use of CONST_CS; I'm not sure about it's definition (wrt. external extensions).
To be honest, I think the majority of the changes are unnecessary, since the CONST_CS usages will occasionally be gone anyway during the migration to stubs.
In my opinion, it's ok to remove the CONST_CS constant itself in PHP 8.3, since PHP 8.2 already started to get rid of most of the usages in php-src at least. In any case, we could add an upgrading note to PHP 8.2 about the deprecation of the flag.
@kocsismate, thanks for review. I didn't realize that you already remove this constant. I close my branch then. :)
No, you don't have to close this pr 🙂 some of the usages can still be removed!
@kocsismate , I'm sorry, but I didn't understand. I thought that my changes are necessary, because you will cover it during your migration to stubs as you did inside this PR: #9731
I'm still not well oriented in PHP source code and I didn't remove all the cases at once. Just the trivial once to not break anything. If you need any help of me with migration then I would be glad to help.
@jorgsowa, the changes directly relating to REGISTER_*_CONSTANT will be covered by the migration by stubs (@kocsismate already covered at least some of these during the last days). But there are some other changes (e.g. the one to ext/com_dotnet) which should be applied on top of that. I'm re-opening this ticket as a reminder. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! This looks mostly good to me (I left some comments), but maybe @kocsismate wants to have a look, too.
Thank you!
Uh oh!
There was an error while loading. Please reload this page.
Hello everyone,
this PR removes the majority of the usage of the
CONST_CSin trivial cases which is not necessary anymore.Kind regards