Cleanup app/etc/di.xml #30886

thomas-kl1 · 2020-11-11T21:50:02Z

Cleanup the app/etc/di.xml that belongs to magento/magento2-base I guess

Move some specific di declaration to the module they belongs to.
Remove some di declaration which were duplicated.
Remove some not necessary di declaration.

Resolved issues:

resolves [Issue] Cleanup app/etc/di.xml #30887: Cleanup app/etc/di.xml

m2-assistant · 2020-11-11T21:50:05Z

Hi @thomas-kl1. 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

❗ Automated tests can be triggered manually with an appropriate comment:

@magento run all tests - run or re-run all required tests against the PR changes
@magento run <test-build(s)> - run or re-run specific test build(s)
For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

Database Compare
Functional Tests CE
Functional Tests EE,
Functional Tests B2B
Integration Tests
Magento Health Index
Sample Data Tests CE
Sample Data Tests EE
Sample Data Tests B2B
Static Tests
Unit Tests
WebAPI Tests

You can find more information about the builds here

ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

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

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket.

🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel

✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

sidolov · 2020-11-11T22:36:24Z

@magento create issue

thomas-kl1 · 2021-01-19T08:33:34Z

@magento run all tests

mrtuvn · 2021-02-05T02:27:55Z

@magento run all tests

mrtuvn · 2021-02-05T17:23:49Z

app/code/Magento/Catalog/etc/di.xml

-            <argument name="state" xsi:type="object" shared="false">Magento\Framework\Mview\View\StateInterface</argument>
-            <argument name="changelog" xsi:type="object" shared="false">Magento\Framework\Mview\View\ChangelogInterface</argument>
-        </arguments>
-    </type>
    <type name="Magento\Catalog\Model\Indexer\Product\Full">


why we need remove this ?

good catch, it shouldn't, didn't seen the shared="false" at first place. I need to investigate the reason to have shared="false"

mrtuvn · 2021-02-05T17:30:42Z

app/etc/di.xml

@@ -1113,7 +1053,7 @@
    </type>
    <type name="Magento\Framework\Session\Validator">
        <arguments>
-            <argument name="scopeType" xsi:type="const">Magento\Store\Model\ScopeInterface::SCOPE_STORE</argument>
+            <argument name="scopeType" xsi:type="const">Magento\Framework\App\ScopeInterface::SCOPE_DEFAULT</argument>
            <argument name="skippedUserAgentList" xsi:type="array">


argument scopeType should be remove due duplicated in module Store already defined

it's overriden in the Magento\Store module, the idea is to separate responsabilities and design scope. app/etc/di.xml should only contain framework di without knowing the context of modules

no Magento_Store will override this file. File app/etc/di.xml will ultilized first after that is module global di.xml

mrtuvn · 2021-02-05T17:36:00Z

app/etc/di.xml

@@ -485,7 +445,7 @@
    <type name="Magento\Framework\Url">
        <arguments>
            <argument name="session" xsi:type="object">Magento\Framework\Session\Generic\Proxy</argument>
-            <argument name="scopeType" xsi:type="const">Magento\Store\Model\ScopeInterface::SCOPE_STORE</argument>
+            <argument name="scopeType" xsi:type="const">Magento\Framework\App\ScopeInterface::SCOPE_DEFAULT</argument>
        </arguments>


should be Magento\Store\Model\ScopeInterface::SCOPE_STORE

https://github.com/magento/magento2/blob/fd59d87babcaf3e1dd189817fec3d8774cb4213c/app/code/Magento/Store/etc/di.xml#L253

it's overriden with proper value in Magento\Store module

mrtuvn · 2021-02-05T18:14:31Z

In my opinion you only need clean file app/etc/di.xml with remove some parts unneccessary (backslash) is enough

thomas-kl1 · 2021-02-05T18:18:13Z

In my opinion you only need clean file app/etc/di.xml with remove some parts unneccessary (backslash) is enough

app/etc/di declarations should only be framework specific. Anything that belongs to a module should be declared as specific in the module di. THe idea behind that is to get magento2 base module agnostic

mrtuvn · 2021-02-09T03:10:02Z

Much better seem ok with me !
One more thing i'm not sure this cleanup effort to have consistency across the platform, as in some places we have backslash, in others we don’t. About leading backslash can you update all other place ?
Let's see final tests results

thomas-kl1 · 2021-02-09T09:56:47Z

Modules I don't want to touch:

Magento\Ui
Magento\Translation
Magento\MessageQueue
Magento\GraphQl

thomas-kl1 · 2021-02-18T16:08:46Z

@magento run all tests

thomas-kl1 · 2022-07-27T20:47:50Z

@magento run all tests

magento-automated-testing · 2022-07-27T20:47:59Z

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

magento-engcom-team added Component: Backend Component: Catalog Component: Config Component: Directory Component: Eav Component: MediaStorage Component: Store Component: Theme Component: WebapiAsync Release Line: 2.4 labels Nov 11, 2020

sidolov added Priority: P3 May be fixed according to the position in the backlog. Severity: S4 Affects aesthetics, professional look and feel, “quality” or “usability”. labels Nov 11, 2020

m2-community-project bot added Progress: pending review labels Nov 11, 2020

m2-assistant bot mentioned this pull request Nov 11, 2020

[Issue] Cleanup app/etc/di.xml #30887

Open

mrtuvn reviewed Feb 5, 2021

View reviewed changes

m2-community-project bot assigned mrtuvn Feb 5, 2021

m2-community-project bot added Progress: review and removed Progress: pending review labels Feb 5, 2021

mrtuvn reviewed Feb 5, 2021

View reviewed changes

thomas-kl1 force-pushed the patch-5 branch from fd59d87 to ffcb267 Compare February 7, 2021 13:55

thomas-kl1 force-pushed the patch-5 branch from 0d0d406 to 1148490 Compare February 9, 2021 09:59

thomas-kl1 added 3 commits July 27, 2022 22:40

cleanup default di.xml

d564872

remove leading backslash

eee0a86

move global declaration from module to app/etc/di.xml

cffca96

thomas-kl1 force-pushed the patch-5 branch from 1148490 to cffca96 Compare July 27, 2022 20:47

Merge branch '2.4-develop' into patch-5

48b8084

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup app/etc/di.xml #30886

Cleanup app/etc/di.xml #30886

thomas-kl1 commented Nov 11, 2020 •

edited by m2-assistant bot

Loading

m2-assistant bot commented Nov 11, 2020

sidolov commented Nov 11, 2020

thomas-kl1 commented Jan 19, 2021

mrtuvn commented Feb 5, 2021

mrtuvn Feb 5, 2021

thomas-kl1 Feb 5, 2021

mrtuvn Feb 5, 2021

thomas-kl1 Feb 5, 2021

mrtuvn Feb 5, 2021 •

edited

Loading

mrtuvn Feb 5, 2021

thomas-kl1 Feb 5, 2021

mrtuvn commented Feb 5, 2021

thomas-kl1 commented Feb 5, 2021

mrtuvn commented Feb 9, 2021 •

edited

Loading

thomas-kl1 commented Feb 9, 2021 •

edited

Loading

thomas-kl1 commented Feb 18, 2021

thomas-kl1 commented Jul 27, 2022

magento-automated-testing bot commented Jul 27, 2022

Cleanup app/etc/di.xml #30886

Are you sure you want to change the base?

Cleanup app/etc/di.xml #30886

Conversation

thomas-kl1 commented Nov 11, 2020 • edited by m2-assistant bot Loading

Cleanup the app/etc/di.xml that belongs to magento/magento2-base I guess

Resolved issues:

m2-assistant bot commented Nov 11, 2020

sidolov commented Nov 11, 2020

thomas-kl1 commented Jan 19, 2021

mrtuvn commented Feb 5, 2021

mrtuvn Feb 5, 2021

Choose a reason for hiding this comment

thomas-kl1 Feb 5, 2021

Choose a reason for hiding this comment

mrtuvn Feb 5, 2021

Choose a reason for hiding this comment

thomas-kl1 Feb 5, 2021

Choose a reason for hiding this comment

mrtuvn Feb 5, 2021 • edited Loading

Choose a reason for hiding this comment

mrtuvn Feb 5, 2021

Choose a reason for hiding this comment

thomas-kl1 Feb 5, 2021

Choose a reason for hiding this comment

mrtuvn commented Feb 5, 2021

thomas-kl1 commented Feb 5, 2021

mrtuvn commented Feb 9, 2021 • edited Loading

thomas-kl1 commented Feb 9, 2021 • edited Loading

thomas-kl1 commented Feb 18, 2021

thomas-kl1 commented Jul 27, 2022

magento-automated-testing bot commented Jul 27, 2022

thomas-kl1 commented Nov 11, 2020 •

edited by m2-assistant bot

Loading

mrtuvn Feb 5, 2021 •

edited

Loading

mrtuvn commented Feb 9, 2021 •

edited

Loading

thomas-kl1 commented Feb 9, 2021 •

edited

Loading