magento-lts icon indicating copy to clipboard operation
magento-lts copied to clipboard

Prepare declare strict_types=1

Open luigifab opened this issue 2 years ago • 6 comments

Description

This PR allow to add <?php declare(strict_types=1); in all files of OpenMage (@sreichel in #3647). This is only the beginning.

To add it in all files, run find app/ shell/ lib/Mage/ lib/Magento/ lib/Varien/ -name "*.ph*" -type f -exec sed -i '1s/<?php/<?php declare(strict_types=1);/' {} +

Apply this PR and enjoy backend.

To cancel changes: find app/ shell/ lib/Mage/ lib/Magento/ lib/Varien/ -name "*.ph*" -type f -exec sed -i '1s/<?php declare(strict_types=1);/<?php/' {} +

OpenMage 20.2.0, PHP 7.2 / 8.3.

Contribution checklist

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

luigifab avatar Nov 12 '23 13:11 luigifab

you need to be very careful with this, there are several situations where it changes the behavior of the code

Flyingmana avatar Nov 12 '23 16:11 Flyingmana

Yes, I'm not sure to understand all possible impacts, but, for localhost it looks good! And, with them, I found some critical fails of the life!

luigifab avatar Nov 12 '23 17:11 luigifab

there are several situations where it changes the behavior of the code

I did not see any changes in code behavoir during my tests. (not finished, but 80%) Either it works, or you see a fatal error. I think its not that hard to add strict types, its just a lot of work and manual testing. (unit tests would be nice here)

sreichel avatar Nov 28 '23 01:11 sreichel

Looks good, perhaps extract enhancement for error message in another PR, and add Mage::logException(). Else we can close it.

luigifab avatar Nov 30 '23 18:11 luigifab

Agree with @kiatng, to speed things up I've removed from this PR the code that was not related to the strict_types thing.

fballiano avatar Feb 07 '24 09:02 fballiano

There's only one point open to fix for this PR.

fballiano avatar Apr 26 '24 10:04 fballiano

since all the comments and subsequent modification, I think this is mergeable

fballiano avatar May 06 '24 08:05 fballiano