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.

Sign up for GitHub

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

Jump to bottom

Deprecate AbstractAction and it's public methods #27964

Merged
merged 5 commits into from
Apr 29, 2020
Merged

Deprecate AbstractAction and it's public methods #27964

merged 5 commits into from
Apr 29, 2020

Conversation

lbajsarowicz
Copy link
Contributor

Description (*)

Abstract Action and Action class should not be used

Related Pull Requests

N/A

Fixed Issues (if relevant)

N/A

Manual testing scenarios (*)

N/A

Questions or comments

CC: @lenaorobei

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
Copy link

m2-assistant bot commented Apr 24, 2020

Hi @lbajsarowicz. 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.4-develop instance - deploy vanilla Magento instance

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

@dmytro-ch dmytro-ch self-assigned this Apr 24, 2020
orlangur
orlangur reviewed Apr 24, 2020
View reviewed changes
lib/internal/Magento/Framework/App/Action/AbstractAction.php Outdated
use Magento\Framework\App\RequestInterface;
use Magento\Framework\App\ResponseInterface;
use Magento\Framework\Controller\Result\RedirectFactory;
use Magento\Framework\Controller\ResultFactory;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please avoid such changes in future pull requests, it needs to be a separate effort rather than noise in regular diffs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sorry about that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed noise for you.

@ghost ghost assigned orlangur and lbajsarowicz Apr 24, 2020
@lbajsarowicz lbajsarowicz force-pushed the bugfix/deprecate-action branch from 467a73b to 1759898 Compare April 24, 2020 08:36
@lbajsarowicz
Copy link
Contributor Author

@orlangur Reverted the "noise" and rebased to hide the traces.

lenaorobei
lenaorobei reviewed Apr 24, 2020
View reviewed changes
lib/internal/Magento/Framework/App/Action/AbstractAction.php Outdated
@@ -56,6 +60,7 @@ public function __construct(
* Retrieve request object
*
* @return \Magento\Framework\App\RequestInterface
* @deprecated This method should not be used anymore. Inject `RequestInterface` into constructor instead
Copy link
Contributor

Choose a reason for hiding this comment

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

We do not deprecate methods, but classes.

@ghost ghost assigned lenaorobei Apr 24, 2020
lenaorobei
lenaorobei reviewed Apr 24, 2020
View reviewed changes
lib/internal/Magento/Framework/App/Action/AbstractAction.php Outdated
abstract class AbstractAction implements \Magento\Framework\App\ActionInterface
/**
* @deprecated Use \Magento\Framework\App\ActionInterface
* @see https://community.magento.com/t5/Magento-DevBlog/Decomposition-of-Magento-Controllers/ba-p/430883
Copy link
Contributor

Choose a reason for hiding this comment

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

Please change to:

/**
 * @deprecated Inheritance was eliminated, composition should be used instead.
 * @see \Magento\Framework\App\ActionInterface
 */

@see tag will allow navigate class in IDE.

lenaorobei
lenaorobei reviewed Apr 24, 2020
View reviewed changes
lib/internal/Magento/Framework/App/Action/Action.php Outdated
@@ -24,6 +24,7 @@
* Action classes that do not extend from this class will lose this behavior and might not function correctly
*
* @deprecated Use \Magento\Framework\App\ActionInterface
* @see https://community.magento.com/t5/Magento-DevBlog/Decomposition-of-Magento-Controllers/ba-p/430883
Copy link
Contributor

Choose a reason for hiding this comment

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

Please change to

/**
 * @deprecated Inheritance was eliminated, composition should be used instead.
 * @see \Magento\Framework\App\ActionInterface
 */

Copy link
Contributor

Choose a reason for hiding this comment

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

I would keep both links, just to have more info

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's forbidden to add links to code

Copy link
Contributor

@ihor-sviziev ihor-sviziev Apr 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@lenaorobei @lbajsarowicz Could you explain where it declared and why?
From my perspective add link that could describe more detailed why class is deprecated and what to use instead is really good option!

I even checked documentation for phpdoc - @see tag could contains URL to documentation https://docs.phpdoc.org/latest/references/phpdoc/tags/see.html

Copy link
Contributor

Choose a reason for hiding this comment

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

@ihor-sviziev we can put links to official documentation only.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got your point. Thx

@lenaorobei lenaorobei added the Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests label Apr 24, 2020
@ghost ghost assigned ihor-sviziev Apr 27, 2020
@ihor-sviziev ihor-sviziev removed their assignment Apr 27, 2020
@slavvka slavvka added this to the 2.4.0 milestone Apr 27, 2020
magento-engcom-team pushed a commit that referenced this pull request Apr 29, 2020
@slavvka
ENGCOM-7491: Deprecate AbstractAction and it's public methods #27964
dfb00d6
@magento-engcom-team magento-engcom-team merged commit a021cff into magento:2.4-develop Apr 29, 2020
@m2-assistant
Copy link

m2-assistant bot commented Apr 29, 2020

Hi @lbajsarowicz, 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ihor-sviziev ihor-sviziev ihor-sviziev left review comments

@orlangur orlangur orlangur left review comments

@lenaorobei lenaorobei lenaorobei approved these changes

Assignees

@orlangur orlangur

@lbajsarowicz lbajsarowicz

@dmytro-ch dmytro-ch

@lenaorobei lenaorobei

Labels
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests Component: App Partner: Mediotype partners-contribution Pull Request is created by Magento Partner Progress: accept Release Line: 2.4
Projects
Pull Requests Dashboard
Archived in project
Milestone
2.4.0
Development

Successfully merging this pull request may close these issues.
7 participants
@lbajsarowicz @orlangur @ihor-sviziev @lenaorobei @slavvka @dmytro-ch @magento-engcom-team