Remove unnecessary usage of CONST_CS by jorgsowa · Pull Request #9685 · php/php-src

jorgsowa · 2022-10-06T22:40:05Z

Hello everyone,
this PR removes the majority of the usage of the CONST_CS in trivial cases which is not necessary anymore.

Kind regards

TysonAndre · 2022-10-07T22:52:53Z

#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

cmb69 · 2022-10-10T14:46:54Z

 	CLSCTX ctx = CLSCTX_SERVER;
 	HRESULT res = E_FAIL;
-	int mode = COMG(autoreg_case_sensitive) ? CONST_CS : 0;
+	int mode = 0;


I wonder if we should just inline that 0.

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.

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.

cmb69 · 2022-10-10T14:48:23Z

Thank you! I'm in favor of dropping the use of CONST_CS; I'm not sure about it's definition (wrt. external extensions).

kocsismate · 2022-10-11T06:03:16Z

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.

jorgsowa · 2022-10-11T21:05:34Z

@kocsismate, thanks for review. I didn't realize that you already remove this constant. I close my branch then. :)

kocsismate · 2022-10-11T21:07:04Z

No, you don't have to close this pr 🙂 some of the usages can still be removed!

jorgsowa · 2022-10-12T18:21:28Z

@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.

cmb69 · 2022-10-13T09:20:05Z

@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. :)

cmb69

Thank you! This looks mostly good to me (I left some comments), but maybe @kocsismate wants to have a look, too.

cmb69 · 2022-11-28T16:12:13Z

Thank you!

Removed unnecessary usage of CONST_CS

141908a

github-actions Bot added Category: Engine Extension: com_dotnet Extension: hash Extension: sodium Extension: standard Extension: xml SAPI: cli labels Oct 6, 2022

TysonAndre requested a review from kocsismate October 7, 2022 22:53

cmb69 reviewed Oct 10, 2022

View reviewed changes

Inline mode value

bf79375

jorgsowa closed this Oct 11, 2022

cmb69 reopened this Oct 13, 2022

kocsismate reviewed Oct 13, 2022

View reviewed changes

Comment thread ext/hash/hash.c

Comment thread ext/sodium/sodium_pwhash.c

Comment thread ext/standard/dir.c

Comment thread ext/standard/password.c

Comment thread main/main.c

Merge branch 'master' into remove-usage-const-cs-1

f3413a4

github-actions Bot removed Category: Engine Extension: xml labels Oct 13, 2022

Removed usage of CONST_CS

f943271

github-actions Bot added Category: Engine Extension: opcache SAPI: phpdbg labels Oct 13, 2022

kocsismate reviewed Oct 14, 2022

View reviewed changes

Comment thread Zend/zend_vm_def.h

Comment thread sapi/cli/php_cli.c Outdated

Comment thread sapi/phpdbg/phpdbg.c Outdated

Merge branch 'master' into remove-usage-const-cs-1

bbaf64e

Removed empty flags functions

2e17efa

kocsismate reviewed Nov 14, 2022

View reviewed changes

Comment thread Zend/zend_builtin_functions.c

jorgsowa added 3 commits November 22, 2022 23:34

Reverted back ZEND_CONSTANT_SET_FLAGS

0c0569b

Reverted setting flags to constants

9ae8fa7

Simplify method php_com_import_typelib

04552b6

cmb69 reviewed Nov 23, 2022

View reviewed changes

Comment thread Zend/zend_builtin_functions.c Outdated

Comment thread Zend/zend_vm_def.h Outdated

Comment thread sapi/cli/php_cli.c

cmb69 reviewed Nov 23, 2022

View reviewed changes

Comment thread ext/com_dotnet/com_typeinfo.c Outdated

jorgsowa added 2 commits November 24, 2022 23:25

Changed indentation

92172e7

Reverted function replacement

601466e

kocsismate approved these changes Nov 26, 2022

View reviewed changes

cmb69 merged commit 77ee92a into php:master Nov 28, 2022

jorgsowa deleted the remove-usage-const-cs-1 branch June 26, 2023 21:25

Uh oh!

Conversation

jorgsowa commented Oct 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TysonAndre commented Oct 7, 2022

Uh oh!

cmb69 Oct 10, 2022

Choose a reason for hiding this comment

Uh oh!

jorgsowa Oct 10, 2022

Choose a reason for hiding this comment

Uh oh!

cmb69 Oct 10, 2022

Choose a reason for hiding this comment

Uh oh!

cmb69 commented Oct 10, 2022

Uh oh!

kocsismate commented Oct 11, 2022

Uh oh!

jorgsowa commented Oct 11, 2022

Uh oh!

kocsismate commented Oct 11, 2022

Uh oh!

jorgsowa commented Oct 12, 2022

Uh oh!

cmb69 commented Oct 13, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cmb69 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cmb69 commented Nov 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jorgsowa commented Oct 6, 2022 •

edited

Loading