MAGETWO-94153: Add functionality to composer update to lookahead for … #1

pdohogne-magento · 2018-10-17T21:58:36Z

…root composer.json changes when updating a magento/product* package

buskamuza · 2018-10-18T22:35:03Z

src/Magento/Composer/Plugin/RootUpdate/RootUpdateCommand.php

+                $eventDispatcher->addListener(
+                    ScriptEvents::POST_UPDATE_CMD,
+                    [$this, 'writeUpdatedRoot'],
+                    PHP_INT_MAX


Shouldn't it be at the place where we write to the file (line 230)?

The listener is preferable, as adding it with PHP_INT_MAX priority will allow us to put it at the head of the event queue and write the composer.json changes before any other POST_UPDATE_CMD scripts are triggered. Line 230 handles the case where ROOT_ONLY_OPT is passed, which doesn't call the native update's execute() method and thus doesn't trigger event listeners but we still want to write the updated composer.json.

buskamuza · 2018-10-18T22:37:21Z

src/Magento/Composer/Plugin/RootUpdate/RootUpdateCommand.php

+            if ($installVal === null) {
+                $action = static::ADD_VAL;
+            } else {
+                $action = 'change';


Should it be static::CHANGE_VAL?

…turn value

buskamuza · 2018-10-24T15:06:50Z

composer.json

+  ],
+  "type": "composer-plugin",
+  "require": {
+    "composer/composer": "*",


Are you sure that any version will work?

paliarush · 2018-11-07T22:33:44Z

LICENSE.txt

@@ -0,0 +1,48 @@
+
+Open Software License ("OSL") v. 3.0


Would be nice to have more info about the project in the Readme.

paliarush · 2018-11-07T22:42:31Z

composer.json

+    }
+  },
+  "extra": {
+    "class": "Magento\\Composer\\Plugin\\RootUpdate\\RootUpdatePlugin"


How exactly the plugin is executed considering that there is no scripts declaration?

Composer supports plugins (the package is of type "composer-plugin"). The "extra->class" section in composer.json tells Composer the class that implements PluginInterface and it goes from there. This is a full plugin, not a script hook, which can't touch commands.

https://getcomposer.org/doc/articles/plugins.md

paliarush · 2018-11-07T23:04:43Z

src/Magento/Composer/Plugin/RootUpdate/RootUpdateCommand.php

+        $updatePrepared = false;
+        $updater = new MagentoRootUpdater($io, $composer, $input);
+        try {
+            // Move the native UpdateCommand's deprecation message before the added Magento functionality


How is this going to be maintained in the future to be in sync with new composer versions? If I understand correctly, we are duplicating some composer logic here.

The same as anything else: with new releases of the plugin. The current composer/composer version constraint in composer.json locks this version of the plugin to the most recent version of Composer or below, which have been tested and are compatible. As new composer versions come out that would affect the plugin's functionality, we'll update it and adjust the composer requirement to fit.

Actually, it's interesting question... I believe the constraint in composer.json is what Composer will download for your plugin to use (as if you'd specify Symfony). So that composer version may be different than Composer running by the user. Interesting how this can be handled. Could you please check what happens if you run composer version different from what's specified in the constraint?

paliarush · 2018-11-07T23:10:10Z

src/Magento/Composer/Plugin/RootUpdate/RootUpdateCommand.php

+        }
+
+        $errorCode = null;
+        if (!$input->getOption(static::ROOT_ONLY_OPT)) {


Changing plugin to require instead of update like suggested by Olga, would allow to eliminate all this logic.

paliarush · 2018-11-07T23:39:52Z

src/Magento/Composer/Plugin/RootUpdate/RootUpdateCommand.php

+
+            if ($errorCode && !$updater->isStrictConstraint()) {
+                $io->writeError(
+                    '<warning>Recommended: Use a specific Magento version constraint instead of "' .


How using strict constraint might fix update errors?

The plugin uses the same version constraint as the composer file to find the target version to check for changes, but it can't do the full dependency compatibility checks that Composer does during the full update (that's what the plugin tries to help solve in the first place), which means that a non-strict constraint could have the plugin using values for a different version than is actually used by Composer, and those values might not be compatible with the real target version. A strict constraint ensures that both the plugin and the full update are using the same target package version.

Is there a way to use composer functionality in some dry-run mode to get the actually resolved version?

The problem is that we need the plugin to fix dependencies in order to find the actual resolved version in order to know what version to use for the dependencies the plugin needs to fix; it's a recursive loop. Official Magento documentation says to use strict version constraints, so rather than over-engineering a solution that's not foolproof, a best-guess approach + a warning on the fail case should be appropriate.

melnikovi · 2018-11-09T19:04:43Z

src/Magento/Composer/Plugin/RootUpdate/RootUpdateCommand.php

+use Symfony\Component\Console\Output\OutputInterface;
+
+/**
+ * Class RootUpdateCommand


It's ok for now, but in general it's better to provide more descriptive comments and we also don't use @Package annotation.

paliarush · 2018-11-09T19:57:35Z

src/Magento/RootUpdatePluginInstaller/WebSetupWizardPluginInstaller.php

+            return;
+        }
+
+        if (!preg_match('/\/var\/composer\.json$/', $path)) {


It is possible that after installation, someone will clear the var directory along with the copied version of plugin. This will lead to silent bugs.

To prevent such situation we can add logic of copying this extension to the new version of web setup wizard, to the same place where it copies composer.json

paliarush · 2018-11-09T20:00:36Z

src/Magento/RootUpdatePluginInstaller/WebSetupWizardPluginInstaller.php

+     */
+    public static function updateSetupWizardPlugin($io, $composer, $path, $version)
+    {
+        $productRegex = '/magento\/product-(community|enterprise)-edition/';


Please wrap each logical piece into a separate function to improve readability. E.g. isMagentoProductRequired and isMagentoComposerUpdatePluginRequired

paliarush · 2018-11-09T21:45:21Z

src/Magento/Composer/Plugin/RootUpdate/MagentoRootUpdater.php

+                $prettyBaseVersion = $fromRoot['version'];
+            }
+
+            $baseRoot = null;


Please rename all variables similar to $baseRoot to $baseRootComposerJsonFile. Similarly $baseRootComposerLockFile if any

paliarush · 2018-11-09T22:58:44Z

src/Magento/Composer/Plugin/RootUpdate/MagentoRootUpdater.php

+     * @param string $minimumStability
+     * @return array
+     */
+    protected function extractStabilityFlags($reqName, $reqVersion, $minimumStability)


Please move this logic to a separate class to separate our custom logic from the one copy-pasted from the composer.

Same relates to logic like deprecation warnings in \Magento\Composer\Plugin\RootUpdate\RootUpdateCommand::execute

…d install command and README.md text

paliarush

Please address new comments.

During this review pass I skipped the logic of the plugin, assuming that it is sufficiently covered with integration tests and has not changed much since previous review.

paliarush · 2019-01-10T16:30:33Z

README.md

+
+In this case, run the following command with the appropriate values to correct the existing `composer.json` file before proceeding with the expected `composer require` command for the target Magento product.
+
+    composer require <current_Magento_package> <current_version> --previous-magento-package <previous_Magento_package>=<previous_Magento_version>


Should it be previous or initial? Because if someone initially installed M 2.0 and then upgraded to 2.3 and now trying to upgrade to 2.3.1, they will have previous = 2.3, initial = 2.0. And their root composer.json will still be from M 2.0.

paliarush · 2019-01-10T16:32:32Z

README.md

+
+To resolve these conflicts interactively, re-run the `composer require` command with the `--interactive-magento-conflicts` option.
+
+To override all conflicting custom values with the expected Magento values, re-run the `composer require` command with the `--use-magento-values` option.


use-magento-values => use-default-magento-values ?

paliarush · 2019-01-10T16:33:25Z

README.md

+To override all conflicting custom values with the expected Magento values, re-run the `composer require` command with the `--use-magento-values` option.
+
+## Bypassing the plugin
+To run the native `composer require` command without the plugin's updates, use the `--skip-magento-root` option.


ski-magento-root => skip-magento-root-plugin ?

paliarush · 2019-01-10T16:36:11Z

src/Magento/ComposerRootUpdatePlugin/ComposerReimplementation/AccessibleRootPackageLoader.php

+ *
+ * Copy and expose necessary private methods
+ *
+ * Functions here may need to be updated to match future versions of Composer


We need to establish some procedure around this to make sure in the future it will be clear for any developer working on this code that these classes must be updated.

paliarush · 2019-01-10T16:38:45Z

src/Magento/ComposerRootUpdatePlugin/LICENSE.txt

@@ -0,0 +1,48 @@
+
+Open Software License ("OSL") v. 3.0


Why do we have two different licenses in here?

I copied the pattern in core Magento, which uses both OSL and AFL.

Please check with our legal team on which licenses should we put before making the repo public.

https://git.corp.adobe.com/OpenSourceAdvisoryBoard/opensource_submission_process/issues/284#issuecomment-1512782

For the recently open-sourced m2-devtools repo, @StevenG from Adobe states that the dual licenses to match existing practices makes sense.

I will be going through the Adobe open-sourcing process for the plugin, see https://jira.corp.magento.com/browse/MAGETWO-94868

paliarush · 2019-01-10T16:40:36Z

src/Magento/ComposerRootUpdatePlugin/Plugin/CommandProvider.php

+use Magento\ComposerRootUpdatePlugin\Plugin\Commands\UpdatePluginNamespaceCommands;
+
+/**
+ * Class CommandProvider


Documentation is now mandatory for all new classes and methods we introduce. BTW, we need to setup builds to run our static tests against this code somehow, or at the very least run the manually.

There should be a build for this and it should be part of the contribution requirements to pass this build.
Please create a task for it and, ideally, get it implemented. If not, at least discuss with PO on when it can be done.

https://jira.corp.magento.com/browse/DEVOPS-4469

paliarush · 2019-01-10T16:43:29Z

src/Magento/ComposerRootUpdatePlugin/README.md

@@ -0,0 +1,52 @@
+# Overview


Why do we duplicate readme here and in the root of the project?

Root of the project for the Github code, in src/Magento/ComposerRootUpdatePlugin for the composer package, which is generated from that directory instead of the root. This is the same as in core Magento modules.

This repo is different from Magento repo. Why we don't put all the code in the root of the repo and so have single readme and any other such files? The package is created from the root of the repo.

The package is created from the src/Magento/ComposerRootUpdatePlugin directory.

The package can't be sourced from the root of the repo because of the integration tests. In order to test the plugin functionality on actual composer files, the tests need to install the plugin as a package in a test directory. However, composer won't allow a package to be installed in a subdirectory of itself, so the src folder where the plugin package exists needs to be separate from the tests folder.

buskamuza · 2019-01-16T05:37:49Z

src/Magento/ComposerRootUpdatePlugin/ComposerReimplementation/AccessibleRootPackageLoader.php

+use Composer\Package\Version\VersionParser;
+
+/**
+ * Class AccessibleRootPackageLoader


This line is not needed.

buskamuza · 2019-01-16T05:38:37Z

src/Magento/ComposerRootUpdatePlugin/ComposerReimplementation/AccessibleRootPackageLoader.php

+/**
+ * Class AccessibleRootPackageLoader
+ *
+ * Copy and expose necessary private methods


private methods "... of Composer library"? As it is right now, doesn't sound clear.

buskamuza · 2019-01-16T06:23:20Z

src/Magento/ComposerRootUpdatePlugin/LICENSE.txt

@@ -0,0 +1,48 @@
+
+Open Software License ("OSL") v. 3.0


Please check with our legal team on which licenses should we put before making the repo public.

buskamuza · 2019-01-16T06:25:47Z

src/Magento/ComposerRootUpdatePlugin/Plugin/CommandProvider.php

+use Magento\ComposerRootUpdatePlugin\Plugin\Commands\UpdatePluginNamespaceCommands;
+
+/**
+ * Class CommandProvider


There should be a build for this and it should be part of the contribution requirements to pass this build.
Please create a task for it and, ideally, get it implemented. If not, at least discuss with PO on when it can be done.

buskamuza · 2019-01-16T06:32:03Z

src/Magento/ComposerRootUpdatePlugin/Plugin/Commands/MageRootRequireCommand.php

+                null,
+                InputOption::VALUE_REQUIRED,
+                'Use a previously-installed Magento product version as the base for composer.json updates',
+                static::PREV_OPT_HINT


Maybe it's better to put into the description? I think it may be confusing that a default value is used as a hint for people who are used to the Symfony console interface.

buskamuza · 2019-01-16T06:33:57Z

src/Magento/ComposerRootUpdatePlugin/Plugin/Commands/MageRootRequireCommand.php

+                static::PREVIOUS_PACKAGE_OPT,
+                null,
+                InputOption::VALUE_REQUIRED,
+                'Use a previously-installed Magento product version as the base for composer.json updates',


"Use ..." implies it is a binary (yes/no) option. At least this is how I read it. It'd rephrase to "Edition and version of Magento product to use as a base ..." - something like this.

Also... how does it look like? --previous-magento-package=magento/product-community-edition=2.2.5? Looks confusing that it has two = in it. Maybe it's better to split into two options?

Also, I'd rename it to --base-... instead of --previous.

buskamuza · 2019-01-16T06:38:53Z

src/Magento/ComposerRootUpdatePlugin/Plugin/Commands/MageRootRequireCommand.php

+    public function execute(InputInterface $input, OutputInterface $output)
+    {
+        $updater = null;
+        Console::setIO($this->getIO());


Why static methods of Console are used here?

Console boils down to a singleton IO implementation which I wanted to avoid passing around to every class that needs it (which is most of them), but it needs the IOInterface that is initially set when the command is called to function. I can change it to a non-static class if preferred, I just thought the code was more readable without passing a Console object to most methods.

buskamuza · 2019-01-16T06:41:55Z

src/Magento/ComposerRootUpdatePlugin/Plugin/Commands/MageRootRequireCommand.php

+
+        if ($didUpdate && $errorCode !== 0) {
+            // If the native execute() didn't succeed, revert the Magento changes to the composer.json file
+            $this->revertMageComposerFile('The native \'composer ' . $this->commandName . '\' command failed');


It would be useful to add some identification of this command. Otherwise for someone who doesn't know how our plugin works it will look very confusing (because he/she wouldn't know about existence of our command).
Maybe something like [magento/composer-root-update-plugin] at the beginning.

I'm not sure what you mean here, can you clarify?

I'm saying that for the user it will be hard to understand that this error is caused by the plugin (vs Composer itself). So the message should include some info that points to it.

buskamuza · 2019-01-16T06:48:37Z

src/Magento/ComposerRootUpdatePlugin/Utils/Console.php

+/**
+ * Class Console
+ *
+ * Shared static logger and interaction methods


Why do we need it as a static class?

Console boils down to a singleton IO implementation which I wanted to avoid passing around to every class that needs it (which is most of them), but it needs the IOInterface that is initially set when the command is called to function. I can change it to a non-static class if preferred, I just thought the code was more readable without passing a Console object to most methods.

buskamuza · 2019-01-16T06:49:48Z

src/Magento/ComposerRootUpdatePlugin/Setup/WebSetupWizardPluginInstaller.php

+/**
+ * Class WebSetupWizardPluginInstaller
+ */
+abstract class WebSetupWizardPluginInstaller


Why is it abstract?

Every method inside is static without any saved values, so it doesn't need instantiation. I made it abstract to make it more clear that there was no need to create new instances of it to use.

abstract means that it must be extended. It's strange to use it this way. We should reconsider the staticness of the class too. Singleton is not a good practice (similar to global vars).

WebSetupWizardPluginInstaller doesn't maintain any state, so there's nothing to instantiate. I can make the class non-abstract, but there's no reason to require the user to instantiate a new instance.

buskamuza · 2019-01-16T07:02:06Z

src/Magento/ComposerRootUpdatePlugin/Setup/WebSetupWizardPluginInstaller.php

+        $packageName = PluginDefinition::PACKAGE_NAME;
+
+        // If in ./var already or Magento or the plugin is missing from composer.json, do not install in var
+        if (!preg_match('/\/composer\.json$/', $filePath) ||


This is a public method, but I don't see that $filePath is validated to make sure it's not including sensitive locations.

What validation would you like?

buskamuza · 2019-01-16T07:06:16Z

src/Magento/ComposerRootUpdatePlugin/etc/module.xml

@@ -0,0 +1,11 @@
+<?xml version="1.0"?>


Why is the plugin declared as a Magento module? Does it have module functionality?

The module functionality allows the plugin to install/update itself inside the var directory for the WebSetupWizard functionality when bin magento setup commands are run. It only uses the module implementation for the InstallData, RecurringData, and UpgradeData classes that Magento hooks into. Without it, the plugin won't be present in var if bin magento setup:uninstall -> bin magento setup:install are run (this was caught by the upgrade verification builds). We discussed this on the call we had before the previous code review.

The new composer magento-update-plugin install command will also do the reinstallation when it is run (and will be added to the WebSetupWizard itself, that code will be in its own PR), but the module implementation makes the plugin more invisible for existing installations that won't have the updated setup wizard.

COMOPS-890: Cloud Support

MAGETWO-94153: Add functionality to composer update to lookahead for …

1d3b0c1

…root composer.json changes when updating a magento/product* package

buskamuza reviewed Oct 18, 2018

View reviewed changes

MAGETWO-94153: Fixing a missed findResolution() static::CHANGE_VAL re…

13baa97

…turn value

buskamuza reviewed Oct 24, 2018

View reviewed changes

composer.json Outdated

],

"type": "composer-plugin",

"require": {

"composer/composer": "*",

Copy link

buskamuza Oct 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure that any version will work?

MAGETWO-94153: Enabling the plugin for Web Setup Wizard upgrades

2e50d40

paliarush reviewed Nov 7, 2018

View reviewed changes

LICENSE.txt

@@ -0,0 +1,48 @@

Open Software License ("OSL") v. 3.0

Copy link

paliarush Nov 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to have more info about the project in the Readme.

paliarush reviewed Nov 7, 2018

View reviewed changes

melnikovi reviewed Nov 9, 2018

View reviewed changes

paliarush reviewed Nov 9, 2018

View reviewed changes

pdohogne-magento added 2 commits December 21, 2018 11:40

MC-5465: Refactoring to use the require command and add WebSetupWizar…

5625792

…d install command and README.md text

Resetting beta version number

54c181f

paliarush suggested changes Jan 10, 2019

View reviewed changes

paliarush approved these changes Jan 11, 2019

View reviewed changes

buskamuza suggested changes Jan 16, 2019

View reviewed changes

buskamuza reviewed Jan 16, 2019

View reviewed changes

pdohogne-magento merged commit 54c181f into magento:develop May 23, 2019

pdohogne-magento deleted the MAGETWO-94153-initial-implementation branch May 23, 2019 21:08

mmansoor-magento pushed a commit that referenced this pull request Dec 16, 2020

Merge pull request #1 from magento-borg/COMOPS-890-cloud-support

5807d6e

COMOPS-890: Cloud Support


		In this case, run the following command with the appropriate values to correct the existing `composer.json` file before proceeding with the expected `composer require` command for the target Magento product.

		composer require <current_Magento_package> <current_version> --previous-magento-package <previous_Magento_package>=<previous_Magento_version>


		To resolve these conflicts interactively, re-run the `composer require` command with the `--interactive-magento-conflicts` option.

		To override all conflicting custom values with the expected Magento values, re-run the `composer require` command with the `--use-magento-values` option.

MAGETWO-94153: Add functionality to composer update to lookahead for … #1

MAGETWO-94153: Add functionality to composer update to lookahead for … #1

Uh oh!

Conversation

pdohogne-magento commented Oct 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

melnikovi Nov 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paliarush left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

melnikovi Nov 9, 2018 •

edited

Loading