ObjectManager cleanup - Remove usage from AdminNotification module #26939

ihor-sviziev · 2020-02-20T08:15:26Z

Description (*)

This PR cleans up object manager usage and marks optional arguments as not optional in non API classes.

Related Pull Requests

Fixed Issues (if relevant)

N/A

Manual testing scenarios (*)

...
...

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 · 2020-02-20T08:15:29Z

Hi @ihor-sviziev. 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.

magento-deployment-service · 2020-02-20T08:15:30Z

Hi @m2-assistant[bot] Here is your new Magento Instance: https://undefined
Admin access: https://undefined/undefined
Login: undefined, Password: undefined

lbajsarowicz

@ihor-sviziev Please fix static tests.
There are minor comments, generally speaking 👍

app/code/Magento/AdminNotification/Block/System/Messages.php

app/code/Magento/AdminNotification/Controller/Adminhtml/Notification/MarkAsRead.php

magento-engcom-team · 2020-02-20T09:36:38Z

Hi @lbajsarowicz, thank you for the review.
ENGCOM-6942 has been created to process this Pull Request
✳️ @lbajsarowicz, could you please add one of the following labels to the Pull Request?

Label	Description
`Auto-Tests: Covered`	All changes in Pull Request is covered by auto-tests
`Auto-Tests: Not Covered`	Changes in Pull Request requires coverage by auto-tests
`Auto-Tests: Not Required`	Changes in Pull Request does not require coverage by auto-tests

Pull Request state was updated. Re-review required.

ihor-sviziev · 2020-02-20T13:01:36Z

@lbajsarowicz could you request the changes in order not to move them to test?

lbajsarowicz

Leaving at "Request changes" for styling the PHP files.

Fix small issues after code review

magento-engcom-team · 2020-02-26T12:11:35Z

Hi @lbajsarowicz, thank you for the review.
ENGCOM-6942 has been created to process this Pull Request
✳️ @lbajsarowicz, could you please add one of the following labels to the Pull Request?

Label	Description
`Auto-Tests: Covered`	All changes in Pull Request is covered by auto-tests
`Auto-Tests: Not Covered`	Changes in Pull Request requires coverage by auto-tests
`Auto-Tests: Not Required`	Changes in Pull Request does not require coverage by auto-tests

engcom-Delta · 2020-02-27T09:34:16Z

✔️ QA passed

Nazar65

Hi @engcom-Kilo it would be great if you will squash commits into one.

Nazar65 · 2020-03-03T14:11:49Z

app/code/Magento/AdminNotification/Controller/Adminhtml/Notification/MassMarkAsRead.php

-     * @return void
+     * @inheritdoc
+     *
+     * @return \Magento\Framework\App\ResponseInterface|\Magento\Framework\Controller\ResultInterface


@return is not required if there already @inheritdoc

Nazar65 · 2020-03-03T14:12:01Z

app/code/Magento/AdminNotification/Controller/Adminhtml/Notification/MassRemove.php

-     * @return void
+     * @inheritdoc
+     *
+     * @return \Magento\Framework\App\ResponseInterface|\Magento\Framework\Controller\ResultInterface


Nazar65 · 2020-03-03T14:12:19Z

app/code/Magento/AdminNotification/Controller/Adminhtml/Notification/Remove.php

-     * @return void
+     * @inheritdoc
+     *
+     * @return \Magento\Framework\App\ResponseInterface|\Magento\Framework\Controller\ResultInterface


…back' into cleanup-objectmanager-fallback # Conflicts: # app/code/Magento/AdminNotification/Controller/Adminhtml/Notification/MarkAsRead.php # app/code/Magento/AdminNotification/Controller/Adminhtml/Notification/MassMarkAsRead.php # app/code/Magento/AdminNotification/Controller/Adminhtml/Notification/MassRemove.php # app/code/Magento/AdminNotification/Controller/Adminhtml/Notification/Remove.php

…ion module #26939

m2-assistant · 2020-03-12T04:27:17Z

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

ihor-sviziev added the Cleanup label Feb 20, 2020

ghost added the Progress: pending review label Feb 20, 2020

magento-engcom-team added Component: AdminNotification Release Line: 2.4 labels Feb 20, 2020

ihor-sviziev mentioned this pull request Feb 20, 2020

[TEST] This PR is for testing only, DO NOT MERGE. #26594

Closed

4 tasks

ihor-sviziev force-pushed the cleanup-objectmanager-fallback branch from 0a36b13 to dfda317 Compare February 20, 2020 08:20

ObjectManager cleanup - Remove usage from AdminNotification module

d359cdf

ihor-sviziev force-pushed the cleanup-objectmanager-fallback branch from dfda317 to d359cdf Compare February 20, 2020 08:21

lbajsarowicz previously approved these changes Feb 20, 2020

View reviewed changes

magento-engcom-team assigned lbajsarowicz Feb 20, 2020

lbajsarowicz suggested changes Feb 20, 2020

View reviewed changes

ObjectManager cleanup - Remove usage from AdminNotification module

744b0f7

Fix small issues after code review

ihor-sviziev requested a review from lbajsarowicz February 26, 2020 12:09

ghost assigned ihor-sviziev Feb 26, 2020

lbajsarowicz approved these changes Feb 26, 2020

View reviewed changes

ghost added Progress: ready for testing and removed Progress: pending review labels Feb 26, 2020

ihor-sviziev removed their assignment Feb 26, 2020

engcom-Delta self-assigned this Feb 27, 2020

ghost added Progress: testing in progress and removed Progress: ready for testing labels Feb 27, 2020

ghost added Progress: extended testing and removed Progress: testing in progress labels Feb 27, 2020

engcom-Kilo self-assigned this Feb 27, 2020

Nazar65 suggested changes Feb 28, 2020

View reviewed changes

Bartlomiejsz mentioned this pull request Mar 3, 2020

Cleanup ObjectManager usage - Magento_AdminNotification #27081

Closed

4 tasks

engcom-Kilo force-pushed the cleanup-objectmanager-fallback branch from 84d80f4 to 91e3afb Compare March 3, 2020 11:53

magento2/pull/26939: Fixed static test.

2598994

engcom-Kilo force-pushed the cleanup-objectmanager-fallback branch from 91e3afb to 2598994 Compare March 3, 2020 11:59

engcom-Kilo requested a review from Nazar65 March 3, 2020 12:41

Nazar65 suggested changes Mar 3, 2020

View reviewed changes

engcom-Kilo added 3 commits March 3, 2020 16:39

magento2/pull/26939: Fixed static test.

57af3d7

magento2/pull/26939: Fixed static test.

b7c1d04

engcom-Kilo requested a review from Nazar65 March 4, 2020 11:23

Nazar65 approved these changes Mar 4, 2020

View reviewed changes

ghost added Progress: accept and removed Progress: extended testing labels Mar 4, 2020

slavvka added the Auto-Tests: Covered All changes in Pull Request is covered by auto-tests label Mar 4, 2020

magento-engcom-team pushed a commit that referenced this pull request Mar 12, 2020

ENGCOM-6942: ObjectManager cleanup - Remove usage from AdminNotificat…

51755fd

…ion module #26939

magento-engcom-team merged commit 480f8ab into magento:2.4-develop Mar 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ObjectManager cleanup - Remove usage from AdminNotification module #26939

ObjectManager cleanup - Remove usage from AdminNotification module #26939

ihor-sviziev commented Feb 20, 2020

m2-assistant bot commented Feb 20, 2020

magento-deployment-service bot commented Feb 20, 2020

lbajsarowicz left a comment •

edited

Loading

magento-engcom-team commented Feb 20, 2020

ihor-sviziev commented Feb 20, 2020

lbajsarowicz left a comment

magento-engcom-team commented Feb 26, 2020

engcom-Delta commented Feb 27, 2020

Nazar65 left a comment •

edited

Loading

Nazar65 Mar 3, 2020

Nazar65 Mar 3, 2020

Nazar65 Mar 3, 2020

m2-assistant bot commented Mar 12, 2020

ObjectManager cleanup - Remove usage from AdminNotification module #26939

ObjectManager cleanup - Remove usage from AdminNotification module #26939

Conversation

ihor-sviziev commented Feb 20, 2020

Description (*)

Related Pull Requests

Fixed Issues (if relevant)

Manual testing scenarios (*)

Questions or comments

Contribution checklist (*)

m2-assistant bot commented Feb 20, 2020

magento-deployment-service bot commented Feb 20, 2020

lbajsarowicz left a comment • edited Loading

Choose a reason for hiding this comment

magento-engcom-team commented Feb 20, 2020

ihor-sviziev commented Feb 20, 2020

lbajsarowicz left a comment

Choose a reason for hiding this comment

magento-engcom-team commented Feb 26, 2020

engcom-Delta commented Feb 27, 2020

Nazar65 left a comment • edited Loading

Choose a reason for hiding this comment

Nazar65 Mar 3, 2020

Choose a reason for hiding this comment

Nazar65 Mar 3, 2020

Choose a reason for hiding this comment

Nazar65 Mar 3, 2020

Choose a reason for hiding this comment

m2-assistant bot commented Mar 12, 2020

lbajsarowicz left a comment •

edited

Loading

Nazar65 left a comment •

edited

Loading