Skip to content
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

Updated lib/Varien for PHP8.1 #2802

Merged
merged 1 commit into from
Dec 25, 2022
Merged

Updated lib/Varien for PHP8.1 #2802

merged 1 commit into from
Dec 25, 2022

Conversation

sreichel
Copy link
Contributor

@sreichel sreichel commented Dec 11, 2022

Description (*)

Related Pull Requests

  1. See add some ReturnTypeWillChange annotations to Varien_object #2301

Fixed Issues (if relevant)

  61     Return type mixed of method Varien_Data_Form_Element_Collection::getIterator() is not covariant with tentative return type Traversable of method IteratorAggregate::getIterator().  
         💡 Make it covariant, or use the #[\ReturnTypeWillChange] attribute to temporarily suppress the error.  
  ...
  ...

Manual testing scenarios (*)

  1. change phpVersion: 80000 to phpVersion: 80100 in phpstan.dist.neon
  2. run it for lib/Varien (other directories are okay)

Questions or comments

@mattdavenport
https://discordapp.com/channels/408199701196963842/748568096989511720/1047907939513544837

This should be fixed with #2301. Can you please check you files?

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)
  • Add yourself to contributors list

@sreichel sreichel added the PHP 8.1 Related to PHP 8.1 label Dec 11, 2022
@github-actions github-actions bot added Component: lib/Varien Relates to lib/Varien Component: lib/* Relates to lib/* labels Dec 11, 2022
@fballiano
Copy link
Contributor

I never used return types but AFAIK they were introduced before php7.3 so they're safe to add for us.

@fballiano fballiano merged commit c92cafc into OpenMage:1.9.4.x Dec 25, 2022
@sreichel sreichel deleted the varien-8.1 branch December 25, 2022 21:05
@m-overlund
Copy link
Contributor

m-overlund commented Apr 13, 2023

Note:
this breaks Firegento Logger (Sentry) and results in HTTP 500
, as the same functions needs to have changed the return type in the module.

Maybe someone has access to update it?

Declaration of FireGento_Logger_Model_Event::offsetUnset($offset) must be compatible with Varien_Object::offsetUnset($offset): void

Declaration of FireGento_Logger_Model_Event::offsetExists($offset) must be compatible with Varien_Object::offsetExists($offset): bool
etc.

Or other modules can be expected to break in the same way

sreichel added a commit to sreichel/openmage-patches that referenced this pull request Apr 27, 2023
@sreichel
Copy link
Contributor Author

@m-overlund Tempory help for composer install ...

composer config --json extra.patches.firegento/logger '{"OM-2802 lib/Varien PHP8.1": "https://raw.githubusercontent.com/sreichel/openmage-patches/main/firegento/logger/OM
-2802.patch"}'
composer install

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: lib/Varien Relates to lib/Varien Component: lib/* Relates to lib/* PHP 8.1 Related to PHP 8.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants