Updated PHPStan to 1.9.13 and ZF1Future to 1.22 #2960

fballiano · 2023-01-17T10:52:17Z

Closes ZF1F update (v 1.22.0) #2956

fballiano · 2023-01-17T10:55:35Z

These patches are not applied correctly anymore:

    https://raw.githubusercontent.com/OpenMage/magento-lts/1.9.4.x/patches/MAG-1.9.3.10.patch (MAG-1.9.3.10)
   Could not apply patch! Skipping. The error was: Cannot apply patch https://raw.githubusercontent.com/OpenMage/magento-lts/1.9.4.x/patches/MAG-1.9.3.10.patch
    https://raw.githubusercontent.com/OpenMage/magento-lts/1.9.4.x/patches/MAG-1.9.4.0.patch (MAG-1.9.4.0)
   Could not apply patch! Skipping. The error was: Cannot apply patch https://raw.githubusercontent.com/OpenMage/magento-lts/1.9.4.x/patches/MAG-1.9.4.0.patch
    https://raw.githubusercontent.com/OpenMage/magento-lts/1.9.4.x/patches/ZF-295.patch (ZF-295 - SUPEE-9652/10570)
   Could not apply patch! Skipping. The error was: Cannot apply patch https://raw.githubusercontent.com/OpenMage/magento-lts/1.9.4.x/patches/ZF-295.patch
    https://raw.githubusercontent.com/OpenMage/magento-lts/1.9.4.x/patches/ZF-296.patch (ZF-296 - Better IIS suport)
   Could not apply patch! Skipping. The error was: Cannot apply patch https://raw.githubusercontent.com/OpenMage/magento-lts/1.9.4.x/patches/ZF-296.patch
    https://raw.githubusercontent.com/OpenMage/magento-lts/1.9.4.x/patches/ZF-297.patch (ZF-297 - Invalid argument type)
   Could not apply patch! Skipping. The error was: Cannot apply patch https://raw.githubusercontent.com/OpenMage/magento-lts/1.9.4.x/patches/ZF-297.patch

sreichel · 2023-01-17T10:57:04Z

See #2956

fballiano · 2023-01-17T10:57:51Z

oops, missed that :-D

sreichel · 2023-01-17T10:58:35Z

Patches can be removed.

fballiano · 2023-01-17T11:01:01Z

done! ❤️

sreichel · 2023-01-17T11:03:10Z

ZF-299 has been merged too. Please check.

fballiano · 2023-01-17T11:04:34Z

it gets applied, i'll check the source code after a call I've to do in 5 min :-)

fballiano · 2023-01-17T11:07:08Z

@sreichel it seems ZF-299 wasn't merged in 1.22, if you check https://github.com/Shardj/zf1-future/blob/release-1.22.0/library/Zend/Date.php it doesn't have any of the code included in the patch.

sreichel · 2023-01-17T11:11:50Z

Wrong file?

https://github.com/Shardj/zf1-future/blob/release-1.22.0/library/Zend/Date/DateObject.php

fballiano · 2023-01-17T11:13:17Z

ZF299 patches Zend/Date.php

fballiano · 2023-01-17T11:14:46Z

mmmm I see what you mean, but is it ok if Zend/Date.php stays without the patch that now has been added upstream to DateObject.php?

sreichel · 2023-01-17T11:23:07Z

Mhhh, my PR targets Zend/Date/DateObject.php ....maybe something went wrong here. I'll check it later.

sreichel · 2023-01-17T11:34:23Z

Just from memory ...

Did magento added this to Zend/Date.php with calling parent method (in Zend_Date_DateObject)?

Quick check lgtm.

fballiano · 2023-01-17T13:35:53Z

right, it extends Zend_Date_DateObject, removing that patch in 1 minute

sreichel · 2023-01-17T13:36:43Z

but is it ok if Zend/Date.php stays without the patch that now has been added upstream to DateObject.php?

class Zend_Date extends Zend_Date_DateObject

Y.

fballiano · 2023-01-17T13:41:00Z

uff, sorry for the many commits, kinda busy in this very moment

fballiano · 2023-01-17T13:47:02Z

I think some of the other patches could be merged by ZF1F (maybe not all sadly), @sreichel are you in direct contact with @Shardj?

sreichel · 2023-01-17T13:54:48Z

No, im not in contact with @Shardj.

There should be three MAG-patches left (ignoring MAG-1.1). I'll add them in time.

OM-patches may need to be reviewed before create PR at ZF1F.

sreichel · 2023-01-17T23:44:39Z

Ignore phpstan error ... (possibly repeating error is fixed with removal of code/core/Zend)

Ill update tomorrow,

fballiano · 2023-01-18T15:14:04Z

@sreichel I wanted to submit patches/MAG-1.9.3.9.patch to zf1 but I've seen that in https://github.com/OpenMage/magento-lts/blame/v20.0.18/lib/Zend/Validate/EmailAddress.php (that file for version 20.0.18) that patch wasn't applied.

Just out of curiousity, how did you find the patches? I tried to find info about the modification itself, to provide it to zf1, but had no luck in finding the right SUPEE with details of the fix :-\

elidrissidev · 2023-01-18T16:25:02Z

Some new phpstan errors popped up, possible related to the patch files removed?

sreichel · 2023-01-18T21:53:55Z

@fballiano i think this patch was applied to core/code/Zend.

fballiano · 2023-01-19T00:03:52Z

merged and cherrypicked to v20

justinbeaty · 2023-01-19T20:51:12Z

There's an issue with deleting the patches in the source tree, because composer.json requires it from:

https://raw.githubusercontent.com/OpenMage/magento-lts/1.9.4.x/patches/MAG-1.9.3.10.patch

But now, that file is 404. So if you have a commit earlier than this one, composer install will fail to apply these patches.

To reproduce:

git checkout 2a8e91dffba41cc45f8a62332b1fa4d8c9e8484d
composer install --no-dev --ignore-platform-reqs

output:

  - Applying patches for shardj/zf1-future                                                           
    https://raw.githubusercontent.com/OpenMage/magento-lts/1.9.4.x/patches/MAG-1.1.1.patch (MAG-1.1.1)                                                                                                     
    https://raw.githubusercontent.com/OpenMage/magento-lts/1.9.4.x/patches/MAG-1.9.3.0.patch (MAG-1.9.3.0)                                                                                                 
    https://raw.githubusercontent.com/OpenMage/magento-lts/1.9.4.x/patches/MAG-1.9.3.7.patch (MAG-1.9.3.7 - SUPEE-10415)
    https://raw.githubusercontent.com/OpenMage/magento-lts/1.9.4.x/patches/MAG-1.9.3.9.patch (MAG-1.9.3.9)
    https://raw.githubusercontent.com/OpenMage/magento-lts/1.9.4.x/patches/MAG-1.9.3.10.patch (MAG-1.9.3.10)                                                                                               
   Could not apply patch! Skipping. The error was: The "https://raw.githubusercontent.com/OpenMage/magento-lts/1.9.4.x/patches/MAG-1.9.3.10.patch" file could not be downloaded (HTTP/1.1 404 Not Found)
    https://raw.githubusercontent.com/OpenMage/magento-lts/1.9.4.x/patches/MAG-1.9.4.0.patch (MAG-1.9.4.0)
   Could not apply patch! Skipping. The error was: The "https://raw.githubusercontent.com/OpenMage/magento-lts/1.9.4.x/patches/MAG-1.9.4.0.patch" file could not be downloaded (HTTP/1.1 404 Not Found)    
    https://raw.githubusercontent.com/OpenMage/magento-lts/1.9.4.x/patches/ZF-295.patch (ZF-295 - SUPEE-9652/10570)
   Could not apply patch! Skipping. The error was: The "https://raw.githubusercontent.com/OpenMage/magento-lts/1.9.4.x/patches/ZF-295.patch" file could not be downloaded (HTTP/1.1 404 Not Found)
    https://raw.githubusercontent.com/OpenMage/magento-lts/1.9.4.x/patches/ZF-296.patch (ZF-296 - Better IIS suport)                                                                                       
   Could not apply patch! Skipping. The error was: The "https://raw.githubusercontent.com/OpenMage/magento-lts/1.9.4.x/patches/ZF-296.patch" file could not be downloaded (HTTP/1.1 404 Not Found)         
    https://raw.githubusercontent.com/OpenMage/magento-lts/1.9.4.x/patches/ZF-297.patch (ZF-297 - Invalid argument type)                                                                                   
   Could not apply patch! Skipping. The error was: The "https://raw.githubusercontent.com/OpenMage/magento-lts/1.9.4.x/patches/ZF-297.patch" file could not be downloaded (HTTP/1.1 404 Not Found)         
    https://raw.githubusercontent.com/OpenMage/magento-lts/1.9.4.x/patches/ZF-299.patch (ZF-299 - Limit mktime() year)                                                                                     
   Could not apply patch! Skipping. The error was: The "https://raw.githubusercontent.com/OpenMage/magento-lts/1.9.4.x/patches/ZF-299.patch" file could not be downloaded (HTTP/1.1 404 Not Found)         
    https://raw.githubusercontent.com/OpenMage/magento-lts/1.9.4.x/patches/OM-918.patch (OM-918 - Add runtime cache to Zend_Locale_Data)                                                                   
    https://raw.githubusercontent.com/OpenMage/magento-lts/1.9.4.x/patches/OM-1081.patch (OM-1081 - Not detecting HTTPS behind a proxy)                                                                    
    https://raw.githubusercontent.com/OpenMage/magento-lts/1.9.4.x/patches/OM-2047.patch (OM-2047 - Pass delimiter char to preg_quote)                                                                     
    https://raw.githubusercontent.com/OpenMage/magento-lts/1.9.4.x/patches/OM-2050.patch (OM-2050 - Prevent checking known date codes)

So far it's not a big problem because we don't have any tag where we use cweagans/composer-patches. But if we were to make a new tag v19.4.22, and then after that delete another patch, some users could end up with patches missing.

My first attempt to fix was to define patches like this:

"patches": {
	"shardj/zf1-future": {
		"MAG-1.1.1": "./patches/MAG-1.1.1.patch",
		"MAG-1.9.3.0": "./patches/MAG-1.9.3.0.patch",
		"MAG-1.9.3.7 - SUPEE-10415": "./patches/MAG-1.9.3.7.patch",
		"MAG-1.9.3.9": "./patches/MAG-1.9.3.9.patch",
		"OM-918 - Add runtime cache to Zend_Locale_Data": "./patches/OM-918.patch",
		"OM-1081 - Not detecting HTTPS behind a proxy": "./patches/OM-1081.patch",
		"OM-2047 - Pass delimiter char to preg_quote": "./patches/OM-2047.patch",
		"OM-2050 - Prevent checking known date codes": "./patches/OM-2050.patch"
	}
}

Which does work on if you clone this repo and composer install, but fails if you install via aydin-hassan/magento-core-composer-installer. This PR needs to be installed for it to work: cweagans/composer-patches#417

So, then we should either reference the patches with the commit hash in the URL:

https://raw.githubusercontent.com/OpenMage/magento-lts/37227a383ce29c1df6342a45e4d48bb839876d37/patches/MAG-1.9.3.10.patch

Or, make a new repo for it, openmage/magento-lts-patches or whatever where the patch files will never be deleted.

sreichel · 2023-01-19T21:03:53Z

We should keep it in mind, but actually it should be no problem (unless your branch isnt up2date).

We have no release including ZF1F that requires these files, but its a good idea to move them outside.

justinbeaty · 2023-01-19T21:06:55Z

It's not a problem now, but will be if we were to tag a release today. Because then the patch files could never be deleted from this repository without breaking that release.

sreichel · 2023-01-19T21:13:44Z

@fballiano can you please add at "patch-repo"?

fballiano · 2023-01-19T23:48:54Z

Like zf1patches? Or composerpatches?

sreichel · 2023-01-19T23:54:36Z

I'd prefer "vendor-patches" (or "composer-patches")

fballiano · 2023-01-20T00:25:54Z

https://github.com/OpenMage/composer-patches

elidrissidev · 2023-01-20T14:00:35Z

For the issue with the deleted files, that could've been solved simply by using the permalink which uses the latest commit id, instead of the one with the branch name:

Still works: https://raw.githubusercontent.com/OpenMage/magento-lts/6b0408fce476a738b7008bd3a4b99921fef2ee80/patches/ZF-299.patch
Doesn't work: https://raw.githubusercontent.com/OpenMage/magento-lts/1.9.4.x/patches/ZF-299.patch

fballiano · 2023-01-20T14:24:21Z

@elidrissidev great idea

justinbeaty · 2023-01-20T14:28:02Z

Note that I did mention that in my comment, So, then we should either reference the patches with the commit hash in the URL: [...]

But, a separate repo is still better IMO, because it could be too easy to modify a patch and forget to update the hash in the same PR. Or you do update the hash, but then the hash changes when you squash. Also, finally, older tagged versions still use the older patch [may be a good or bad thing].

fballiano · 2023-01-20T14:29:23Z

the fact that the old tagged version would use the older patch I think it would be a "must have"

justinbeaty · 2023-01-20T15:19:07Z

I'm not against using commit hashes. I guess modifying or introducing a patch need to be done in one PR, and referencing the patch in composer.json needs to be done in a second PR? I think at least...

It would be nice if this was merged instead: cweagans/composer-patches#417

elidrissidev · 2023-01-24T08:29:56Z

It would be nice if this was merged instead: cweagans/composer-patches#417

That would be the best way to do it, but the activity on that repo is very slow, so I have to agree with the current way for now.

Updated PHPStan 1.9.12 and ZF1F 1.22

5afbec7

github-actions bot added the composer Relates to composer.json label Jan 17, 2023

removed unneeded patches

790ddc2

updated composerlock

1abeaf2

fballiano added 3 commits January 17, 2023 13:37

removed ZF299

8387290

updated composerlock

56aa151

file deleted

1d826f1

sreichel previously approved these changes Jan 17, 2023

View reviewed changes

fballiano requested review from ADDISON74, elidrissidev and kiatng January 18, 2023 15:12

phpstan 1.9.13

dc42a7c

fballiano dismissed sreichel’s stale review via dc42a7c January 18, 2023 15:48

fballiano changed the title ~~Updated PHPStan to 1.9.12 and ZF1Future to 1.22~~ Updated PHPStan to 1.9.13 and ZF1Future to 1.22 Jan 18, 2023

ADDISON74 previously approved these changes Jan 18, 2023

View reviewed changes

phpstan.dist.baseline.neon

0f85794

sreichel dismissed ADDISON74’s stale review via 0f85794 January 18, 2023 22:05

github-actions bot added Component: Core Relates to Mage_Core phpstan labels Jan 18, 2023

sreichel approved these changes Jan 18, 2023

View reviewed changes

ADDISON74 approved these changes Jan 18, 2023

View reviewed changes

fballiano merged commit 8d0828e into OpenMage:1.9.4.x Jan 19, 2023

fballiano deleted the update_deps branch January 19, 2023 00:03

fballiano added a commit that referenced this pull request Jan 19, 2023

Updated PHPStan to 1.9.13, ZF1Future to 1.22 (#2960)

14c8fcd

Updated PHPStan to 1.9.13 and ZF1Future to 1.22 #2960

Updated PHPStan to 1.9.13 and ZF1Future to 1.22 #2960

Conversation

fballiano commented Jan 17, 2023 • edited by sreichel

fballiano commented Jan 17, 2023

sreichel commented Jan 17, 2023

fballiano commented Jan 17, 2023

sreichel commented Jan 17, 2023

fballiano commented Jan 17, 2023

sreichel commented Jan 17, 2023

fballiano commented Jan 17, 2023

fballiano commented Jan 17, 2023

sreichel commented Jan 17, 2023

fballiano commented Jan 17, 2023

fballiano commented Jan 17, 2023

sreichel commented Jan 17, 2023

sreichel commented Jan 17, 2023

fballiano commented Jan 17, 2023

sreichel commented Jan 17, 2023

fballiano commented Jan 17, 2023

fballiano commented Jan 17, 2023

sreichel commented Jan 17, 2023

sreichel commented Jan 17, 2023

fballiano commented Jan 18, 2023

elidrissidev commented Jan 18, 2023

sreichel commented Jan 18, 2023

fballiano commented Jan 19, 2023

justinbeaty commented Jan 19, 2023

sreichel commented Jan 19, 2023

justinbeaty commented Jan 19, 2023

sreichel commented Jan 19, 2023

fballiano commented Jan 19, 2023

sreichel commented Jan 19, 2023 • edited

fballiano commented Jan 20, 2023

elidrissidev commented Jan 20, 2023 • edited

fballiano commented Jan 20, 2023

justinbeaty commented Jan 20, 2023

fballiano commented Jan 20, 2023

justinbeaty commented Jan 20, 2023

elidrissidev commented Jan 24, 2023

fballiano commented Jan 17, 2023 •

edited by sreichel

sreichel commented Jan 19, 2023 •

edited

elidrissidev commented Jan 20, 2023 •

edited