magento/magento2#23847: Command "setup:db-declaration:generate-patch" does not add revert() method to patch #23848

atwixfirster · 2019-07-24T09:47:41Z

Description (*)

Fixed Issues (if relevant)

Command "setup:db-declaration:generate-patch" does not add revert() method to patch #23847: Command "setup:db-declaration:generate-patch" does not add revert() method to patch

Manual testing scenarios (*)

Example with Magento_Cms:

bin/magento setup:db-declaration:generate-patch --revertable=true Magento_Cms TestPatch

Actual result:
app/code/Magento/Cms/Setup/Patch/Data/TestPatch.php is created with content without revert() method:

<?php
/**
 * Copyright © Magento, Inc. All rights reserved.
 * See COPYING.txt for license details.
 */

namespace Magento\Cms\Setup\Patch\Data;

use Magento\Framework\Setup\Patch\DataPatchInterface;
use Magento\Framework\Setup\Patch\SchemaPatchInterface;
use Magento\Framework\Setup\Patch\PatchRevertableInterface;
use Magento\Framework\Setup\ModuleDataSetupInterface;

/**
* Patch is mechanism, that allows to do atomic upgrade data changes
*/
class TestPatch implements
    DataPatchInterface
{
    /**
     * @var ModuleDataSetupInterface $moduleDataSetup
     */
    private $moduleDataSetup;

    /**
     * @param ModuleDataSetupInterface $moduleDataSetup
     */
    public function __construct(ModuleDataSetupInterface $moduleDataSetup)
    {
        $this->moduleDataSetup = $moduleDataSetup;
    }

    /**
     * Do Upgrade
     *
     * @return void
     */
    public function apply()
    {
    }

    /**
     * {@inheritdoc}
     */
    public function getAliases()
    {
        return [];
    }

    /**
     * {@inheritdoc}
     */
    public static function getDependencies()
    {
        return [

        ];
    }
}

Expected result:
app/code/Magento/Cms/Setup/Patch/Data/TestPatch.php is created with revert() method in the content:

<?php
/**
 * Copyright © Magento, Inc. All rights reserved.
 * See COPYING.txt for license details.
 */

namespace Magento\Cms\Setup\Patch\Data;

use Magento\Framework\Setup\Patch\DataPatchInterface;
use Magento\Framework\Setup\Patch\SchemaPatchInterface;
use Magento\Framework\Setup\Patch\PatchRevertableInterface;
use Magento\Framework\Setup\ModuleDataSetupInterface;

/**
* Patch is mechanism, that allows to do atomic upgrade data changes
*/
class TestPatch implements
    DataPatchInterface, PatchRevertableInterface
{
    /**
     * @var ModuleDataSetupInterface $moduleDataSetup
     */
    private $moduleDataSetup;

    /**
     * @param ModuleDataSetupInterface $moduleDataSetup
     */
    public function __construct(ModuleDataSetupInterface $moduleDataSetup)
    {
        $this->moduleDataSetup = $moduleDataSetup;
    }

    /**
     * Do Upgrade
     *
     * @return void
     */
    public function apply()
    {
    }

    /**
     * @inheritdoc
     */
    public function revert()
    {
    }

    /**
     * {@inheritdoc}
     */
    public function getAliases()
    {
        return [];
    }

    /**
     * {@inheritdoc}
     */
    public static function getDependencies()
    {
        return [

        ];
    }
}

Questions or comments

Contribution checklist (*)

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds are green)

m2-assistant · 2019-07-24T09:47:43Z

Hi @atwixfirster. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento give me test instance - deploy test instance based on PR changes
@magento give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

dmytro-ch

@atwixfirster, nice catch!
Could you please check the minor CR notices?
Please also check the automated test failures.

Thank you!

dmytro-ch · 2019-07-30T18:56:12Z

app/code/Magento/Developer/Console/Command/GeneratePatchCommand.php

@@ -105,6 +95,7 @@ protected function execute(InputInterface $input, OutputInterface $output) : int
    {
        $moduleName = $input->getArgument(self::MODULE_NAME);
        $patchName = $input->getArgument(self::INPUT_KEY_PATCH_NAME);
+        $includeRevertMethod = ($input->getOption(self::INPUT_KEY_IS_REVERTABLE) == "true");


Please use strict comparison operator ===.
Please also use single-quotes 'true' to keep the same style within the same class unless three is no specific need to use double-quotes.
I would also recommend avoiding the usage of redundant parenthesis.

As for the implementation itself, I think, it should be possible to use the following condition:

if ($input->getOption(self:: INPUT_KEY_IS_REVERTABLE)) { ... }

Please check other examples of \Symfony\Component\Console\Input\InputInterface::getOption method usage.

dmytro-ch · 2019-07-30T19:16:49Z

app/code/Magento/Developer/Console/Command/patch_template.php.dist

@@ -15,7 +15,7 @@ use Magento\Framework\Setup\ModuleDataSetupInterface;
 * Patch is mechanism, that allows to do atomic upgrade data changes
 */
 class %class% implements
-    %patchInterface%
+    %patchInterface%%patchRevertableInterface%


I guess the %patchRevertableInterface% should be on a new line.
Here's an example of what I mean:

/** * Wrapper for GraphQl NonNull */ class NonNull extends \GraphQL\Type\Definition\NonNull implements WrappedTypeInterface, InputTypeInterface, OutputTypeInterface {

I guess the %patchRevertableInterface% should be on a new line.

Not really, @dmytro-ch :)

%patchRevertableInterface% is the alias of , PatchRevertableInterface.

So, with the --revertable=true flag a result will be:

class TestPatch implements DataPatchInterface, PatchRevertableInterface {

Without it:

class TestPatch implements DataPatchInterface {

So, I would like to leave this part as it is.

Thank you!

I mean that the PatchRevertableInterface should be added on a new line.
Please, check the example above, it shows how the interfaces should be arranged.

According to PSR-2 coding style guide:

Lists of implements MAY be split across multiple lines, where each subsequent line is indented once. When doing so, the first item in the list MUST be on the next line, and there MUST be only one interface per line.

I mean that the PatchRevertableInterface should be added on a new line.

done

Command 1:

bin/magento setup:db-declaration:generate-patch --revertable=true Magento_Cms TestPatch

Bash output 1:

Patch app/code/Magento/Cms/Setup/Patch/Data/TestPatch.php has been successfully generated.

Result 1:

<?php /** * Copyright © Magento, Inc. All rights reserved. * See COPYING.txt for license details. */ namespace Magento\Cms\Setup\Patch\Data; use Magento\Framework\Setup\ModuleDataSetupInterface; use Magento\Framework\Setup\Patch\DataPatchInterface; use Magento\Framework\Setup\Patch\PatchRevertableInterface; /** * Patch is mechanism, that allows to do atomic upgrade data changes */ class TestPatch implements DataPatchInterface, PatchRevertableInterface { /** * @var ModuleDataSetupInterface $moduleDataSetup */ private $moduleDataSetup; /** * @param ModuleDataSetupInterface $moduleDataSetup */ public function __construct(ModuleDataSetupInterface $moduleDataSetup) { $this->moduleDataSetup = $moduleDataSetup; } /** * Do Upgrade * * @return void */ public function apply() { } /** * @inheritdoc */ public function revert() { } /** * {@inheritdoc} */ public function getAliases() { return []; } /** * {@inheritdoc} */ public static function getDependencies() { return [ ]; } }

Command 2:

bin/magento setup:db-declaration:generate-patch --revertable=false Magento_Cms TestPatch

Bash output 2:

Patch app/code/Magento/Cms/Setup/Patch/Data/TestPatch.php has been successfully generated.

Result 2:

<?php /** * Copyright © Magento, Inc. All rights reserved. * See COPYING.txt for license details. */ namespace Magento\Cms\Setup\Patch\Data; use Magento\Framework\Setup\ModuleDataSetupInterface; use Magento\Framework\Setup\Patch\DataPatchInterface; /** * Patch is mechanism, that allows to do atomic upgrade data changes */ class TestPatch implements DataPatchInterface { /** * @var ModuleDataSetupInterface $moduleDataSetup */ private $moduleDataSetup; /** * @param ModuleDataSetupInterface $moduleDataSetup */ public function __construct(ModuleDataSetupInterface $moduleDataSetup) { $this->moduleDataSetup = $moduleDataSetup; } /** * Do Upgrade * * @return void */ public function apply() { } /** * {@inheritdoc} */ public function getAliases() { return []; } /** * {@inheritdoc} */ public static function getDependencies() { return [ ]; } }

Command 3:

bin/magento setup:db-declaration:generate-patch --revertable=true --type=schema Magento_Cms TestPatch

Bash output 3:

Patch app/code/Magento/Cms/Setup/Patch/Schema/TestPatch.php has been successfully generated.

Result 3:

<?php /** * Copyright © Magento, Inc. All rights reserved. * See COPYING.txt for license details. */ namespace Magento\Cms\Setup\Patch\Schema; use Magento\Framework\Setup\ModuleDataSetupInterface; use Magento\Framework\Setup\Patch\PatchRevertableInterface; use Magento\Framework\Setup\Patch\SchemaPatchInterface; /** * Patch is mechanism, that allows to do atomic upgrade data changes */ class TestPatch implements SchemaPatchInterface, PatchRevertableInterface { /** * @var ModuleDataSetupInterface $moduleDataSetup */ private $moduleDataSetup; /** * @param ModuleDataSetupInterface $moduleDataSetup */ public function __construct(ModuleDataSetupInterface $moduleDataSetup) { $this->moduleDataSetup = $moduleDataSetup; } /** * Do Upgrade * * @return void */ public function apply() { } /** * @inheritdoc */ public function revert() { } /** * {@inheritdoc} */ public function getAliases() { return []; } /** * {@inheritdoc} */ public static function getDependencies() { return [ ]; } }

Command 4:

bin/magento setup:db-declaration:generate-patch --revertable=false --type=schema Magento_Cms TestPatch

Bash output 4:

Patch app/code/Magento/Cms/Setup/Patch/Schema/TestPatch.php has been successfully generated.

Result 4:

<?php /** * Copyright © Magento, Inc. All rights reserved. * See COPYING.txt for license details. */ namespace Magento\Cms\Setup\Patch\Schema; use Magento\Framework\Setup\ModuleDataSetupInterface; use Magento\Framework\Setup\Patch\SchemaPatchInterface; /** * Patch is mechanism, that allows to do atomic upgrade data changes */ class TestPatch implements SchemaPatchInterface { /** * @var ModuleDataSetupInterface $moduleDataSetup */ private $moduleDataSetup; /** * @param ModuleDataSetupInterface $moduleDataSetup */ public function __construct(ModuleDataSetupInterface $moduleDataSetup) { $this->moduleDataSetup = $moduleDataSetup; } /** * Do Upgrade * * @return void */ public function apply() { } /** * {@inheritdoc} */ public function getAliases() { return []; } /** * {@inheritdoc} */ public static function getDependencies() { return [ ]; } }

atwixfirster · 2019-07-31T12:27:14Z

Could you please check the minor CR notices?

fixed

dmytro-ch · 2019-08-13T17:57:02Z

@atwixfirster, thank you for the fixes!
Could you please squash all commits into a single one in order to clean up history?

m2-assistant · 2019-08-13T20:49:36Z

Hi @atwixfirster, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

… add revert() method to patch

m2-assistant · 2019-08-13T20:53:30Z

Hi @atwixfirster. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento give me test instance - deploy test instance based on PR changes
@magento give me 2.3-develop instance - deploy vanilla Magento instance

In case you'd like to rerun tests use the following comments:

@magento run all tests - run all tests
@magento run { check name } - run a single test job (check name example: Static Tests build)

For more details, please, review the Magento Contributor Guide documentation.

ghost · 2019-08-13T20:54:10Z

@atwixfirster unfortunately, only members of the maintainers team are allowed to assign developers to the pull request

atwixfirster · 2019-08-13T20:55:51Z

Could you please squash all commits into a single one in order to clean up history?

done

Thank you, @dmytro-ch 👍

ghost · 2019-08-13T20:57:08Z

@atwixfirster unfortunately, only members of the maintainers team are allowed to assign developers to the pull request

magento-engcom-team · 2019-08-14T06:07:42Z

Hi @dmytro-ch, thank you for the review.
ENGCOM-5619 has been created to process this Pull Request

engcom-Alfa · 2019-08-19T13:58:54Z

Hi @atwixfirster

During testing we faced the issue

Problem: revert() method generate to patch even if option --revertable=false

Steps to reproduce:
Example with Magento_Cms

bin/magento setup:db-declaration:generate-patch --revertable=false Magento_Cms TestPatch

Actual Result:

class TestPatch implements
DataPatchInterface,
PatchRevertableInterface
{
/**
* @var ModuleDataSetupInterface $moduleDataSetup
*/
private $moduleDataSetup;

/**
 * @param ModuleDataSetupInterface $moduleDataSetup
 */
public function __construct(ModuleDataSetupInterface $moduleDataSetup)
{
    $this->moduleDataSetup = $moduleDataSetup;
}

/**
 * Do Upgrade
 *
 * @return void
 */
public function apply()
{
}

/**
 * @inheritdoc
 */
public function revert()
{
}

/**
 * {@inheritdoc}
 */
public function getAliases()
{
    return [];
}

/**
 * {@inheritdoc}
 */
public static function getDependencies()
{
    return [

    ];
}

@atwixfirster Could you take a look, please?

Thanks!

atwixfirster · 2019-08-19T14:10:06Z

@engcom-Alfa

Problem: revert() method generate to patch even if option --revertable=false

Let me please explain.

If --revertable is present in the bin/magento setup:db-declaration:generate-patch command then revert() method will be added.

So, the key here is --revertable. It may have any value. For example,

--revertable=hello.

Please let me know you you have any additional questions here.

engcom-Alfa · 2019-08-19T14:17:28Z

✔️ QA Passed

Before:

After:

…es not add revert() method to patch #23848

m2-assistant · 2019-08-21T22:47:01Z

Hi @atwixfirster, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

magento-engcom-team added Component: Developer Release Line: 2.3 Partner: Atwix Pull Request is created by partner Atwix partners-contribution Pull Request is created by Magento Partner labels Jul 24, 2019

atwixfirster mentioned this pull request Jul 24, 2019

Command "setup:db-declaration:generate-patch" does not add revert() method to patch #23847

Closed

dmytro-ch self-assigned this Jul 24, 2019

dmytro-ch self-requested a review July 24, 2019 11:18

dmytro-ch requested changes Jul 30, 2019

View reviewed changes

ghost added the Progress: needs update label Jul 30, 2019

atwixfirster closed this Aug 13, 2019

atwixfirster force-pushed the 23847-generate-patch-add-revert-method branch from b84e3b7 to 2a62f1c Compare August 13, 2019 20:49

magento#23847: Command "setup:db-declaration:generate-patch" does not…

84527e7

… add revert() method to patch

atwixfirster reopened this Aug 13, 2019

ghost unassigned dmytro-ch Aug 13, 2019

atwixfirster self-assigned this Aug 13, 2019

ghost unassigned atwixfirster Aug 13, 2019

atwixfirster self-assigned this Aug 13, 2019

ghost unassigned atwixfirster Aug 13, 2019

dmytro-ch self-assigned this Aug 14, 2019

dmytro-ch added Award: special achievement Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests labels Aug 14, 2019

dmytro-ch approved these changes Aug 14, 2019

View reviewed changes

ghost added Progress: ready for testing and removed Progress: needs update labels Aug 14, 2019

engcom-Alfa self-assigned this Aug 19, 2019

ghost added Progress: testing in progress and removed Progress: ready for testing labels Aug 19, 2019

ghost added Progress: needs update and removed Progress: testing in progress labels Aug 19, 2019

ghost added Progress: testing in progress and removed Progress: needs update labels Aug 19, 2019

ghost added Progress: extended testing and removed Progress: testing in progress labels Aug 19, 2019

engcom-Foxtrot self-assigned this Aug 20, 2019

ghost added Progress: accept and removed Progress: extended testing labels Aug 20, 2019

magento-engcom-team pushed a commit that referenced this pull request Aug 21, 2019

ENGCOM-5619: #23847: Command "setup:db-declaration:generate-patch" do…

5a13a0f

…es not add revert() method to patch #23848

magento-engcom-team merged commit 84527e7 into magento:2.3-develop Aug 21, 2019

magento-engcom-team added this to the Release: 2.3.4 milestone Aug 21, 2019

magento/magento2#23847: Command "setup:db-declaration:generate-patch" does not add revert() method to patch #23848

magento/magento2#23847: Command "setup:db-declaration:generate-patch" does not add revert() method to patch #23848

Uh oh!

Conversation

atwixfirster commented Jul 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description (*)

Fixed Issues (if relevant)

Manual testing scenarios (*)

Questions or comments

Contribution checklist (*)

Uh oh!

m2-assistant bot commented Jul 24, 2019

Uh oh!

dmytro-ch left a comment

Choose a reason for hiding this comment

Uh oh!

dmytro-ch Jul 30, 2019

Choose a reason for hiding this comment

Uh oh!

atwixfirster Jul 31, 2019

Choose a reason for hiding this comment

Uh oh!

dmytro-ch Jul 30, 2019

Choose a reason for hiding this comment

Uh oh!

atwixfirster Jul 30, 2019

Choose a reason for hiding this comment

Uh oh!

dmytro-ch Aug 8, 2019 • 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!

atwixfirster Aug 8, 2019 • 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!

atwixfirster commented Jul 31, 2019

Uh oh!

dmytro-ch commented Aug 13, 2019

Uh oh!

m2-assistant bot commented Aug 13, 2019

Uh oh!

m2-assistant bot commented Aug 13, 2019

Uh oh!

ghost commented Aug 13, 2019

Uh oh!

atwixfirster commented Aug 13, 2019

Uh oh!

ghost commented Aug 13, 2019

Uh oh!

magento-engcom-team commented Aug 14, 2019

Uh oh!

engcom-Alfa commented Aug 19, 2019

Uh oh!

atwixfirster commented Aug 19, 2019

Uh oh!

engcom-Alfa commented Aug 19, 2019

Uh oh!

m2-assistant bot commented Aug 21, 2019

Uh oh!

Uh oh!

atwixfirster commented Jul 24, 2019 •

edited

Loading

dmytro-ch Aug 8, 2019 •

edited

Loading

atwixfirster Aug 8, 2019 •

edited

Loading