netz98 / n98-magerun

The swiss army knife for Magento developers, sysadmins and devops. The tool provides a huge set of well tested command line commands which save hours of work time. All commands are extendable by a module API.
http://magerun.net/
Other
1.44k stars 400 forks source link

[WIP-PHP81] Checklist #1275

Closed sreichel closed 1 year ago

sreichel commented 1 year ago

Based on https://github.com/cmuench/n98-magerun/tree/feature/php81

Available commands:

admin :heavy_check_mark:

composer - removed

sreichel commented 1 year ago

For next PR ...

https://github.com/cmuench/n98-magerun/pull/87

Tasks

Errors

addison74 commented 1 year ago

A real work to be appreciated to keep this tool up to date which was heavily used during Magento 1. OpenMage continues Magento 1 and doesn't stop at being an LTS.

sreichel commented 1 year ago

@cmuench PR is ready.

Did not touch composer related commands. Do we still need them?

Install/Uninstall needs to be tested by someone else (does not work in my DDEV environment)

Unittests have to be adjusted. Some help would be nice here :)

cmuench commented 1 year ago

@sreichel I do not work with Magento 1 anymore on a daily base. Maybe we should remove the Composer library from Magerun 1. Removing means that we have to replace also the internal downloader logic which is using the Composer packages. I already did that for Magerun 2 to prevent conflicts with Symfony components etc.

sreichel commented 1 year ago

It would have been easier to backport magerun2 :rofl:

Install/CI fails b/c composer dowload does not work. @cmuench can we/you backport install cmd from magerun2?

sreichel commented 1 year ago

For next PR ...

Backport from n89-magerun2 ...

Updates

Tasks

Errors

cmuench commented 1 year ago

@sreichel I tested the installation on my local machine and it works in the provided ddev environment. Currently I don't know why it's failing in CI. Did the install fail on your machine?

sreichel commented 1 year ago

@cmuench package download does not work for me too. Install dir magento is created with subdir vendor/compoer, but there are no magento files ...

In InstallCommand.php line 1046:

  Installation failed (Exit code 1). Could not open input file: /var/www/html/magento/install.php  
cmuench commented 1 year ago

@cmuench ok, interesting. We will find the issue :-)

cmuench commented 1 year ago

@sreichel I can reproduce it! That's good.

cmuench commented 1 year ago

@sreichel I guess Composer 2.2 is not compatible. I will try to backport the downloader logic from Magerun 2.

cmuench commented 1 year ago

@sreichel I started to backport the installer command. It requires also the backport of the sub-command logic. My WIP is pushed to the branch.

The main issue is that the installer of n98-magerun2 works stricly with Composer packages. The config in the config.yaml looks like this:

  N98\Magento\Command\Installer\InstallCommand:
    magento-packages:
      - name: mage-os-2.4.5-p1
        package: magento/project-community-edition
        version: 2.4.5-p1
        options:
          repository-url: https://mirror.mage-os.org

In n98-magerun1 we have a different structure:

  N98\Magento\Command\Installer\InstallCommand:
    magento-packages:
      - name: openmage-20.0.14
        version: 20.0.14
        dist:
          url: https://github.com/OpenMage/magento-lts/archive/v20.0.14.zip
          type: zip
          shasum: 15172228925d2f03db00712e591f14b15210bea1
        extra:
          sample-data: sample-data-1.9.2.4

Currently we use the internal Composer library to handle that. Sadly I do not see a good way to call an external Composer binary and feed it with the information of the package. I could replace the logic with a simple download and extract, but this would break a lot of automations. If you have a good idea. Let me know.

sreichel commented 1 year ago

@cmuench can we use https://github.com/firegento/magento for composer installable packages instead of magento-mirror?

cmuench commented 1 year ago

@sreichel Installer is now working in the Github Actions. Next a lot of tests have to be fixed. Currently 43 tests are failing.

cmuench commented 1 year ago

Seems that most of the tests are currently not compatible with the new console "ask" method.

sreichel commented 1 year ago

@cmuench thanks for working on this.

Unfortunately my ddev env has some problems ... https://stackoverflow.com/q/76366191/5703627

My changes are very similar to magerun2, may we can reuse some code for unit tests?

sreichel commented 1 year ago

@cmuench we also have to fix sample data intstall.

I can create a repo from @Vinai (or my own) sample data to allow install via composer.

sreichel commented 1 year ago

Next a lot of tests have to be fixed.

@cmuench Working on this next days. Will also update to phpunit 9.

alexherbs commented 1 year ago

Hello,

i wanted to mention following errors which occured during switching from OpenMage PHP7.4 to PHP8.1, maybe its useful for your checklist (n98-magerun --version: 2.3.0):

2023-08-15T03:32:21+00:00 ERR (3): Deprecated functionality: Return type of N98\Magento\Modules::getIterator() should either be compatible with IteratorAggregate::getIterator(): Traversable, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in phar:///usr/bin/n98-magerun.phar/src/N98/Magento/Modules.php on line 96

2023-08-15T03:32:21+00:00 ERR (3): Deprecated functionality: Return type of N98\Magento\Modules::count() should either be compatible with Countable::count(): int, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in phar:///usr/bin/n98-magerun.phar/src/N98/Magento/Modules.php on line 108

2023-08-15T06:05:29+00:00 ERR (3): Deprecated functionality: explode(): Passing null to parameter #2 ($string) of type string is deprecated in phar:///usr/bin/n98-magerun.phar/src/N98/Magento/Command/System/Url/ListCommand.php on line 83

cmuench commented 1 year ago

@sreichel Spent some time to fix the tests and already did the upgrade to PHPUnit 9. I had to skip some tests because too much changed in PHPUnit which needs a lot of more time to refactor. Most of the tests are running fine.

cmuench commented 1 year ago

@sreichel I merged the current state to develop branch. I did this to get the Github Actions running. The old Ubuntu Version in the Github Actions was not supported anymore.

cmuench commented 1 year ago

@sreichel Phar build is also fixed and files.magerun.net is updated with the latest dev version if n98-magerun1. This means that we can now test in a real OpenMage installation. Sadly I have no real OpenMage project to test.

cmuench commented 1 year ago

@sreichel sample data "no" should work in the next minutes. Pushed a fix to develop branch.

cmuench commented 1 year ago

@sreichel You ask about we can handle the DB port... I used port 3306 in the container in the install command. This is OK inside the container. The dynamic port numbers are only for the port forwording to the host. I use the db container and create a separate db for each instance. That works with the PhpStorm ddev integration plugin without any issue.

cmuench commented 1 year ago

@sreichel I added functional tests to the Github Actions. This helps to find issues which cannot be found in the Unit Tests. We test Magento 1.9 and OpenMage 20.0.20.

cmuench commented 1 year ago

@sreichel I will extend the OpenMage Build with a build matrix. I test it in this PR #1297. It will also test with MySQL 5.7 and MySQL 8.0

addison74 commented 1 year ago

@sreichel - Could you please allocate some time to complete this checklist? For everyone who uses OpenMage it would be a gift to be able to continue using n98-magerun. We could also add some custom commands to DDEV as you wanted.

cmuench commented 1 year ago

@sreichel @ADDISON74 The develop version is already "green" in all tests. Feel free to test it in a real project. I sadly have not any real OpenMage project.

sreichel commented 1 year ago

@ADDISON74 i will update during next days.

sreichel commented 1 year ago

@cmuench how about to drop support for php < 8.1 for v3?

v2.3 works up to php 8.0, right? We dont need any backward compatibilty and could make use of php 8.1 features.

sreichel commented 1 year ago

@cmuench work on some cleanup ... (and phpstan L9)

cmuench commented 1 year ago

@sreichel thanks for working on the finalization of the release. About dropping the PHP 7.4 support. We should wait with that. I am very conservative with that. The n98-magerun2 version is still compatible with 7.4. There are a lot of automatization downloading and executing n98-magerun. We need to plan and announce this.

In the EE command should be a check. We can add a constraint to current the PHP version there and skip the tests

The M2 related switches are to provide a hint if the wrong version is used. I think after 10 years we can remove that.

sreichel commented 1 year ago

@cmuench do we need to support something below php8.1 for v3?

For php7-8.0 you can still use v2.3.

For a php8.1 release we can update to symfony 6.4.

EE commands can be moved into an add-on. I know there are commands the are EE-only snare disabled for CE, but others that have different behavior inside the command. This causes some problems with phpstan, that can't find EE-related classes.

However ... I have to split my work into smaller PRs ... hope it's okay to not doing it in one big PR.

cmuench commented 1 year ago

@sreichel The plan is that the current release is still compatible with the latest Magento Open Source 1.9 with PHP 7.4. Let's bring first the build in production. I want to stay on Symfony LTS releases which is currently 5.4. Next LTS is 6.4. The 6.3 release will have the EOL soon.

sreichel commented 1 year ago

Okay. :)

What about EE-commands? Is it fine to move them into a separate repo?

(already removed that code for phpstan and have to revert it ...)

cmuench commented 1 year ago

@sreichel They will never be called and registered in OpenMage due to this code:

 public function isEnabled()
 {
     return $this->getApplication()->isMagentoEnterprise();
 }

We can just ignore them and remove the test setup. I am defensive, because we do not know if someone is still running Magento EE. I guess it's a license problem if someone does. I am not sure that the best procedure here is. It's also fine to drop the support for Magento EE here.

sreichel commented 1 year ago

@cmuench i know, but im sure there are commands that work for EE and CE, but have different behavior inside the command. (e.g. create admin user - adding roles)

cmuench commented 1 year ago

We can drop support for EE this release. And deprecate then the old Magento CE 1.9 for the next major release. Than we can prepare the people and make the tool only compatible with OpenMage.

sreichel commented 1 year ago

Okay ... PRs incoming next days :)

sreichel commented 1 year ago

@sreichel - Could you please allocate some time to complete this checklist? For everyone who uses OpenMage ...

Maybe some OM users want to join for help? At least with tests?

sreichel commented 1 year ago

@cmuench killed my work with composer require ... ... LOL

I'll try to split my PRs into small chunks ... sorry for extra work, but maybe PRs are much more clear.

sreichel commented 1 year ago

@cmuench think we can close this.

All my changes in pipeline are only code style improvments. No functional changes.

I'll provide updates step by step, but i thinks its ready to make a php81 release.