RE: Added \ReturnTypeWillChange Attribute - (#120) #141

partikus · 2022-10-14T06:33:30Z

This PR updates #120 and switches to zf1s/polyfill-php81.

All credits goes to @jack-worman 🥇

- Added \ReturnTypeWillChange Attribute - Added \ReturnTypeWillChange polyfill - Fixed tests

Required jworman/polyfill-php81

…ompat

falkenhawk

there are more files with unnecessary whitespace changes, but I stopped listing them.

falkenhawk · 2022-10-14T07:34:07Z

composer.json

+    },
+    "config": {
+        "sort-packages": true


I think we don't need it here. BTW, I wonder, why it is not true by default, at least starting from composer v2 🤡

removed in #146

falkenhawk · 2022-10-14T07:35:04Z

packages/zend-acl/composer.json

@@ -5,6 +5,7 @@
    "homepage": "http://framework.zend.com/",
    "license": "BSD-3-Clause",
    "require": {
+        "zf1s/polyfill-php81": "^1.25",


but could you rearrange requires in individual package so that they are sorted alphabetically, please?
And please make sure polyfill-php81 is required only when it is really needed for a package, i.e. when any of the following are used inside the package.

array_is_list

MYSQLI_REFRESH_REPLICA constant

ReturnTypeWillChange

But after a short research, it might be that we don't need the polyfill at all? First two features are not used anywhere, if I'm not mistaken, and ReturnTypeWillChange stub would be only needed for php 8.0, IF it would throw an error on it. But it does not throw an error, either for that or any attribute which is not implemented, e.g. #[Gibberish] would not throw any error even if class Gibberish is not implemented as an attribute.

Maybe it would be needed for code analysis, running on php 8.0. Even in that case, it would be probably enough, if added to the main composer.json only. But we do not have any code analyzers running here, and if one dev would like to run it locally, why would they run it on php 8.0?

Please check if those polyfills are really needed. thank you

Addressing in #146

Appears dependency update not needed, all composer changes can be dropped.

Add zf1s/polyfill-php81 dependencies #146 (comment)

so all left to do is to carry those attribute adding right?

i will remove the root namespace qualifier in the process:

https://github.com/zf1s/zf1/pull/141/files#r1026413789

falkenhawk · 2022-10-14T07:58:01Z

packages/zend-cloud/library/Zend/Cloud/Infrastructure/ImageList.php

-     * 
+     *


unnecessary whitespace change

falkenhawk · 2022-10-14T07:59:33Z

packages/zend-mail/library/Zend/Mail/Part.php

-    
+
    /**


unnecessary whitespace changes in this file

falkenhawk · 2022-10-14T07:59:47Z

packages/zend-navigation/library/Zend/Navigation/Container.php

-     * @throws Zend_Navigation_Exception                    if $pages is not 
+     * @throws Zend_Navigation_Exception                    if $pages is not


more whitespace changes

falkenhawk · 2022-10-14T08:00:53Z

packages/zend-service-amazon/library/Zend/Service/Amazon/Sqs.php

-        
+


falkenhawk · 2022-10-14T08:03:02Z

tests/Zend/Application/Resource/SessionTest.php

+     * Test currently disabled because it freezes phpunit.
+     *
     * @runInSeparateProcess
     */
-    public function testSetOptions()
+    public function _testSetOptions()


maybe skipped tests can be restored now? Phpunit has been adjusted.

falkenhawk · 2022-10-14T08:05:14Z

tests/Zend/Captcha/FigletTest.php

@@ -297,10 +297,12 @@ public function testIsValidShouldNotRequireValueToBeNestedArray()
    }

    /**
+     * Test currently disabled because it freezes phpunit.


maybe skipped tests can be restored now? Phpunit has been adjusted.

…ompat

glensc · 2022-10-17T14:06:22Z

Why adding merge commits to pull request? can't you rebase to flatten commits (without the merge commit)?

partikus · 2022-10-17T19:31:46Z

Thanks @glensc! There are 2 reasons:

did not want to rewrite @jack-worman's commits (well-known issue)
there will be more changes soon

I'm fine with both strategies. After getting comments from @falkenhawk I think rewritting history will be necessary cause some changes are part of @jack-worman's commits. So adding too many cleanup commits will causing an extra mess.

glensc · 2022-10-17T19:49:02Z

@partikus I think rebasing is fine, the author date and author name are still visible with git show, also github somewhat shows them out as well. also I've used Co-authored-by: git trailer when I actually modified original commits to indicate multiple authors. it's in github docs as well:

https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors

glensc · 2022-11-18T13:04:58Z

Looks like nobody of original authors is dealing with the review notes. I might attempt to extract changes from here to separate PR for easier review and greater merge chance.

glensc · 2022-11-18T13:07:00Z

packages/zend-cloud/library/Zend/Cloud/DocumentService/Document.php

@@ -161,6 +161,7 @@ public function __set($name, $value)
     * @param  string $name
     * @return bool
     */
+    #[\ReturnTypeWillChange]


As ZF1 doesn't use namespaces, the root namespace qualifier can be dropped

falkenhawk · 2022-11-18T13:22:48Z

Looks like nobody of original authors is dealing with the review notes. I might attempt to extract changes from here to separate PR for easier review and greater merge chance.

hi @glensc thanks for stepping in. We talked privately with @partikus and he said that he could continue on this PR within two weeks but I can see you are motivated enough to finalize it sooner. 👍

falkenhawk · 2022-11-23T15:18:59Z

replaced with #147

jack-worman and others added 5 commits January 17, 2022 08:17

Added \ReturnTypeWillChange Attribute

b99f9a6

- Added \ReturnTypeWillChange Attribute - Added \ReturnTypeWillChange polyfill - Fixed tests

Required jworman/polyfill-php81

f54a13d

Required jworman/polyfill-php81

More ReturnTypeWillChange

04c1fa8

Merge remote-tracking branch 'upstream/master' into feature/php-8-1-c…

ef63603

…ompat

feat: switching to zf1s/polyfill-php81

ffee1e0

partikus requested review from falkenhawk and marcing October 14, 2022 07:16

falkenhawk reviewed Oct 14, 2022

View reviewed changes

falkenhawk mentioned this pull request Oct 14, 2022

Added \ReturnTypeWillChange Attribute #120

Closed

Merge remote-tracking branch 'upstream/master' into feature/php-8-1-c…

b2611ac

…ompat

falkenhawk mentioned this pull request Nov 7, 2022

Add missing @throws annotations #140

Merged

This was referenced Nov 18, 2022

Add AllowDynamicProperties Attribute to classes #145

Closed

Fix str_split('') logic to keep same as PHP before 8.2 on PHP 8.2 #143

Merged

glensc reviewed Nov 18, 2022

View reviewed changes

glensc mentioned this pull request Nov 18, 2022

Add zf1s/polyfill-php81 dependencies #146

Closed

3 tasks

falkenhawk mentioned this pull request Nov 23, 2022

php 8.1 compat: ReturnTypeWillChange Attribute #147

Merged

falkenhawk closed this Nov 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RE: Added \ReturnTypeWillChange Attribute - (#120) #141

RE: Added \ReturnTypeWillChange Attribute - (#120) #141

partikus commented Oct 14, 2022 •

edited

Loading

falkenhawk left a comment

falkenhawk Oct 14, 2022 •

edited

Loading

glensc Nov 18, 2022

falkenhawk Oct 14, 2022

glensc Nov 18, 2022

glensc Nov 18, 2022

falkenhawk Oct 14, 2022

falkenhawk Oct 14, 2022

falkenhawk Oct 14, 2022

falkenhawk Oct 14, 2022

falkenhawk Oct 14, 2022

falkenhawk Oct 14, 2022

glensc commented Oct 17, 2022

partikus commented Oct 17, 2022

glensc commented Oct 17, 2022

glensc commented Nov 18, 2022

glensc Nov 18, 2022

falkenhawk commented Nov 18, 2022

falkenhawk commented Nov 23, 2022

		* @throws Zend_Navigation_Exception if $pages is not
		* @throws Zend_Navigation_Exception if $pages is not

RE: Added \ReturnTypeWillChange Attribute - (#120) #141

RE: Added \ReturnTypeWillChange Attribute - (#120) #141

Conversation

partikus commented Oct 14, 2022 • edited Loading

falkenhawk left a comment

Choose a reason for hiding this comment

falkenhawk Oct 14, 2022 • 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

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

glensc commented Oct 17, 2022

partikus commented Oct 17, 2022

glensc commented Oct 17, 2022

glensc commented Nov 18, 2022

Choose a reason for hiding this comment

falkenhawk commented Nov 18, 2022

falkenhawk commented Nov 23, 2022

partikus commented Oct 14, 2022 •

edited

Loading

falkenhawk Oct 14, 2022 •

edited

Loading