Fix deprecated error in PHP 8.1/8.2 #1474

koriym · 2023-09-04T17:51:39Z

This PR fixes two deprecated errors in PHP 8.1/8.2.

PHP Deprecated: Creation of dynamic property Swagger\Context::{$prop} is deprecated in swagger-php/src/Context.php on line 53
Deprecated: strcasecmp(): Passing null to parameter #1 ($string1) of type string is deprecated in swagger-php/src/Context.php on line 288

No more errors when doing composer test in PHP 8.1/8.2.

DerManoMann · 2023-09-04T21:03:09Z

Could you please explain how you got these warnings? Just running the tests on PHP 8.1 does not show any warnings for me at all.

Also, I'd rather we fix the cause of the warning instead of just allowing dynamic properties if possible.

koriym · 2023-09-05T04:04:56Z

I did just normal composer update; composer test run.
The logs are as follows.

PHP 8.1

 % composer test
> phpunit && phpcs -p --extensions=php --standard=PSR2 --error-severity=1 --warning-severity=0 ./src ./tests
PHPUnit 9.6.11 by Sebastian Bergmann and contributors.

.................................PHP Deprecated:  strcasecmp(): Passing null to parameter #1 ($string1) of type string is deprecated in /Users/akihito/git/swagger-php/src/Context.php on line 285
PHP Deprecated:  strcasecmp(): Passing null to parameter #2 ($string2) of type string is deprecated in /Users/akihito/git/swagger-php/src/Context.php on line 285
PHP Deprecated:  strcasecmp(): Passing null to parameter #1 ($string1) of type string is deprecated in /Users/akihito/git/swagger-php/src/Context.php on line 285
PHP Deprecated:  strcasecmp(): Passing null to parameter #2 ($string2) of type string is deprecated in /Users/akihito/git/swagger-php/src/Context.php on line 285
.
Deprecated: strcasecmp(): Passing null to parameter #1 ($string1) of type string is deprecated in /Users/akihito/git/swagger-php/src/Context.php on line 285

Deprecated: strcasecmp(): Passing null to parameter #2 ($string2) of type string is deprecated in /Users/akihito/git/swagger-php/src/Context.php on line 285

Deprecated: strcasecmp(): Passing null to parameter #1 ($string1) of type string is deprecated in /Users/akihito/git/swagger-php/src/Context.php on line 285
...

PHP 8.2

Creation of dynamic property error occurs only in PHP 8.2.

 % composer test
> phpunit && phpcs -p --extensions=php --standard=PSR2 --error-severity=1 --warning-severity=0 ./src ./tests
PHPUnit 9.6.11 by Sebastian Bergmann and contributors.

PHP Deprecated:  Creation of dynamic property Swagger\Context::$filename is deprecated in /Users/akihito/git/swagger-php/src/Context.php on line 249
PHP Deprecated:  Creation of dynamic property Swagger\Context::$line is deprecated in /Users/akihito/git/swagger-php/src/Context.php on line 252
PHP Deprecated:  Creation of dynamic property Swagger\Context::$method is deprecated in /Users/akihito/git/swagger-php/src/Context.php on line 256
PHP Deprecated:  Creation of dynamic property Swagger\Context::$class is deprecated in /Users/akihito/git/swagger-php/src/Context.php on line 263
PHP Deprecated:  Creation of dynamic property Swagger\Context::$namespace is deprecated in /Users/akihito/git/swagger-php/src/Context.php on line 265
PHP Deprecated:  Creation of dynamic property Swagger\Context::$comment is deprecated in /Users/akihito/git/swagger-php/src/Analyser.php on line 84
PHP Deprecated:  Creation of dynamic property Swagger\Context::$annotations is deprecated in /Users/akihito/git/swagger-php/src/Analyser.php on line 89
...

koriym · 2023-09-05T04:09:04Z

The Context class assumes dynamic properties by design, and the current master branch also has a [#[\AllowDynamicProperties] attribute.

I think this is not an error suppression, but a design declaration.

DerManoMann · 2023-09-06T02:06:49Z

Sorry, missed that this on the 2.x branch... I shouldn't try handling issues/PRs on my phone, I suppose...

DerManoMann · 2023-09-06T02:16:46Z

src/Annotations/Get.php

@@ -9,6 +9,7 @@
 /**
 * @Annotation
 */
+#[\AllowDynamicProperties]


This seems unrelated to the warnings documented?

Without AllowDynamicProperties attribute, This code causes a deprecate error. I thought I did not need this assignment ($this->$property = $value;), but I gave the attribute to minimize the changes for the behavior unchanged at all.

If I can get rid of this code, I don't need the AllowDynamicProperties attribute. (Is this unexpected property?) What should I do?

Interesting. I have to admit that seem not to be able to reproduce the warnings; that would make everything a bit easier...

I am also confused. I suspect my environment and have had others try, but the error is still there. (I would expect the error to occur since the code is dealing with a dynamic propertyn in PHP 8.1.) However, the GH action I performed in another PR did not reproduce the deprecated error...

:) I think you have custom error logging settings. I would have thought this should be default for phpunit 🤷

php -d error_reporting=-1 ./bin/phpunit

... on it.

Its this test: https://github.com/zircote/swagger-php/blob/2.x/tests/AbstractAnnotationTest.php#L19

Doesn't exist on master any more, otherwise we'd see the same there...

Not quite sure what to do - technically we should probably change the code to throw an exception rather than logging and moving on; just not sure if that is too much of a BC break.
OTOH, this is the first time I've seen this issue, so I would suspect there are not a lot of cases of this happeing out in the wild. On top of that thoses properties are either ignored during serialization or bu any other OpenAPI tools.

I would lean towards not allowing invalid specs to be created, so throwing an exception might be the way forward.

I agree with you that it is correct to throw an exception as you think. But in any case, is there any need to set invalid properties? (I pushed a new commit)

Could we remove this again? When not setting the property this should not be needed.

DerManoMann

One more text change and I think this is good to go.

src/Annotations/AbstractAnnotation.php

Co-authored-by: Martin Rademacher <[email protected]>

DerManoMann · 2023-09-08T01:32:09Z

Did you look at the PR #1476 ?

I actually changed field to property, but more importantly there is a test to update too (that expects this particular message)

This reverts commit 4650139.

DerManoMann · 2023-09-11T02:21:40Z

I think there was some misunderstanding. The change to not set the property was fine. Only thing to change was that in my PR i also slightly changed the log message....

src/Annotations/AbstractAnnotation.php

DerManoMann · 2023-09-11T02:23:25Z

src/Annotations/Get.php

@@ -9,6 +9,7 @@
 /**
 * @Annotation
 */
+#[\AllowDynamicProperties]


Could we remove this again? When not setting the property this should not be needed.

Co-authored-by: Martin Rademacher <[email protected]>

This reverts commit 5384875.

DerManoMann · 2023-09-12T22:12:39Z

Thanks for your patience @koriym

koriym added 2 commits September 5, 2023 02:32

Add #[\AllowDynamicProperties]

db2c53b

Fix type deprecation

05c60de

DerManoMann requested changes Sep 6, 2023

View reviewed changes

Do not set unexpected property

4650139

DerManoMann requested changes Sep 7, 2023

View reviewed changes

src/Annotations/AbstractAnnotation.php Outdated Show resolved Hide resolved

Update log message: Ignoring unexpected field

b4224e0

Co-authored-by: Martin Rademacher <[email protected]>

Revert "Do not set unexpected property"

5384875

This reverts commit 4650139.

DerManoMann requested changes Sep 11, 2023

View reviewed changes

koriym and others added 3 commits September 11, 2023 12:47

Update src/Annotations/AbstractAnnotation.php

c1120fc

Co-authored-by: Martin Rademacher <[email protected]>

Revert "Revert "Do not set unexpected property""

751cbe6

This reverts commit 5384875.

Fix expected error log message

a1ac338

DerManoMann approved these changes Sep 12, 2023

View reviewed changes

DerManoMann merged commit cc88b23 into zircote:2.x Sep 12, 2023
15 checks passed

koriym deleted the php8.2-compat branch September 13, 2023 01:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix deprecated error in PHP 8.1/8.2 #1474

Fix deprecated error in PHP 8.1/8.2 #1474

koriym commented Sep 4, 2023 •

edited by DerManoMann

Loading

DerManoMann commented Sep 4, 2023

koriym commented Sep 5, 2023

koriym commented Sep 5, 2023 •

edited

Loading

DerManoMann commented Sep 6, 2023

DerManoMann Sep 6, 2023

koriym Sep 6, 2023 •

edited

Loading

DerManoMann Sep 6, 2023

koriym Sep 6, 2023

DerManoMann Sep 6, 2023

DerManoMann Sep 6, 2023

koriym Sep 7, 2023

DerManoMann Sep 11, 2023

DerManoMann left a comment

DerManoMann commented Sep 8, 2023

DerManoMann commented Sep 11, 2023

DerManoMann Sep 11, 2023

DerManoMann commented Sep 12, 2023

Fix deprecated error in PHP 8.1/8.2 #1474

Fix deprecated error in PHP 8.1/8.2 #1474

Conversation

koriym commented Sep 4, 2023 • edited by DerManoMann Loading

DerManoMann commented Sep 4, 2023

koriym commented Sep 5, 2023

koriym commented Sep 5, 2023 • edited Loading

DerManoMann commented Sep 6, 2023

Choose a reason for hiding this comment

koriym Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DerManoMann left a comment

Choose a reason for hiding this comment

DerManoMann commented Sep 8, 2023

DerManoMann commented Sep 11, 2023

Choose a reason for hiding this comment

DerManoMann commented Sep 12, 2023

koriym commented Sep 4, 2023 •

edited by DerManoMann

Loading

koriym commented Sep 5, 2023 •

edited

Loading

koriym Sep 6, 2023 •

edited

Loading