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.

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

Remove deprecated framework interfaces #32328

Open
wants to merge 9 commits into
base: 2.5-develop
Choose a base branch
from

Conversation

vlmed
Copy link
Contributor

@vlmed vlmed commented Mar 2, 2021

Description (*)

I created a separate commits for each interface from issue. Since each interface had many dependencies that needed to be edited. In my opinion, it will be easier to review PR.
What done:
1. ilb/internal/Magento/Framework/App/Action/HttpHeadActionInterface.php
- HttpHeadActionInterface was removed.
- All dependency have been fixed.
2. lib/internal/Magento/Framework/App/TemplateTypesInterface.php
- This interface implements 5 classes end extends Magento\Framework\Mail\TemplateInterface. And it has comment
@deprecated 101.0.0 because of incorrect location.
1613144979862

According to the dependencies of lib/internal/Magento/Framework/App/TemplateTypesInterface.php and comment I moved this interface to Magento\Framework\Mail namespace instead of removing.

3. lib/internal/Magento/Framework/Image/Adapter/UploadConfigInterface.php
- UploadConfigInterface has been removed.
- Magento\Framework\Image\Adapter\Config implements this interface. So I removed unused deprecated methods getMaxWidth() and getMaxHeight() and unused deprecated contants from this class.
- All dependencies have been fixed.

1613145732802
1613145772493

4. lib/internal/Magento/Framework/Module/Output/ConfigInterface.php
- ConfigInterface has been removed;
- Magento\Framework\Module\Output\Config is deprecated and implements this interface. So I removed it too. This class is using only in the class Magento\Framework\Module\Manage.
- I changed the behaviour of the deprecated method Magento\Framework\Module\Manage::isOutputEnabled. I removed the code belonging to the class Magento\Framework\Module\Output\Config from the isOutputEnabled method.

1613146610292

5. lib/internal/Magento/Framework/Option/ArrayInterface.php
- ArrayInterface.php has been removed;
- All dependencies have been fixed;

6. lib/internal/Magento/Framework/Session/SidResolverInterface.php
- SidResolverInterface has been removed;
- Magento\Framework\Session\SidResolver is deprecated and implements this interface. SIDs in URLs are no longer used. So I removed this class too.
- I removed Magento\Framework\Session\SidResolver from all classes where it was

**7. lib/internal/Magento/Framework/View/Asset/Bundle/ConfigInterface.php
- ConfigInterface has been removed.
- I also removed deprecated unused classes that are associated with each other and this interface:
- Magento\Framework\View\Asset\Bundle\Config
- Magento\Framework\View\Asset\Bundle\Manager
- Magento\Framework\View\Asset\Bundle
- Magento\Framework\View\Test\Unit\Asset\Bundle\ManagerTest
- Magento\Framework\View\Test\Unit\Asset\BundleTest

8. lib/internal/Magento/Framework/View/Element/UiComponent/Config/ManagerInterface.php
- ManagerInterface has been removed;
- Magento\Ui\Model\Manager is deprecated and implements this interface. So I removed it too. This class is using only in the class Magento\Framework\View\Element\UiComponentFactory.
- Some constants from ManagerInterface are using in the class Magento\Framework\View\Element\UiComponentFactory. So i moved them to the class UiComponentFactory
- All dependencies has been fixed;
-
9. lib/internal/Magento/Framework/Filesystem/ExtendedDriverInterface.php
- ExtendedDriverInterface - I tried to delete it and move the getMetadata() method to DriverInterface.
- But the classes implementing DriverInterface were broken. Because this requires the implementation of the getMetadata() method.
I think we should open a separate issue regarding the removal (or investigate possibility of deleting) of this interface

Created issue: #32331

10. lib/internal/Magento/Framework/MessageQueue/ConfigInterface.php
In most cases this interface is only passed in the constructor and is not used.
But there are a few places where it or its constants are still used.
Used in:

  • Magento\MessageQueue\Console\ConsumerListCommand
  • Magento\Framework\MessageQueue\Code\Generator\Config\RemoteServiceReader\MessageQueue
  • Magento\Framework\MessageQueue\Config\Consumer\ConfigReaderPlugin
  • Magento\Framework\MessageQueue\Config\Publisher\ConfigReaderPlugin
  • Magento\Framework\MessageQueue\Config\Reader\Env\Validator
  • Magento\Framework\MessageQueue\Config\Reader\Xml\Converter\TopicConfig
  • Magento\Framework\MessageQueue\Config\Topology\ConfigReaderPlugin

I think we should open a separate issue regarding the removal (or investigate possibility of deleting) of this interface

Created issue: #32333

Related Pull Requests

https://github.com/magento/partners-magento2ee/pull/512
https://github.com/magento/partners-magento2b2b/pull/558
magento/inventory#3294
magento/magento2-page-builder#735
https://github.com/magento/magento2-page-builder-ee/pull/187

Fixed Issues (if relevant)

  1. Fixes Remove deprecated framework interfaces #32062

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

m2-assistant bot commented Mar 2, 2021

Hi @vlmed. Thank you for your contribution
Here are 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.5-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:

  1. Database Compare
  2. Functional Tests CE
  3. Functional Tests EE,
  4. Functional Tests B2B
  5. Integration Tests
  6. Magento Health Index
  7. Sample Data Tests CE
  8. Sample Data Tests EE
  9. Sample Data Tests B2B
  10. Static Tests
  11. Unit Tests
  12. WebAPI Tests
  13. Semantic Version Checker

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

@magento-engcom-team magento-engcom-team added Partner: Atwix Pull Request is created by partner Atwix partners-contribution Pull Request is created by Magento Partner labels Mar 2, 2021
@m2-community-project m2-community-project bot added the Priority: P1 Once P0 defects have been fixed, a defect having this priority is the next candidate for fixing. label Mar 2, 2021
@vlmed
Copy link
Contributor Author

vlmed commented Mar 2, 2021

@magento run all tests

@vlmed
Copy link
Contributor Author

vlmed commented Mar 2, 2021

Hello @coderimus, @sivaschenko
I created separate issues for:

Please check and help choose the right way to solve them
Thanks

@gabrieldagama gabrieldagama added this to the 2.5 milestone Mar 3, 2021
@bgorski bgorski self-requested a review March 6, 2021 22:22
/**
* The key arguments in the data component
*/
const COMPONENT_ARGUMENTS_KEY = 'arguments';
Copy link
Contributor

Choose a reason for hiding this comment

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

Please explicitly specify visibility of this const.

/**
* The key attributes in the data component
*/
const COMPONENT_ATTRIBUTES_KEY = 'attributes';
Copy link
Contributor

Choose a reason for hiding this comment

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

Please explicitly specify visibility of this const.

/**
* Session ID in query param
*/
const SESSION_ID_QUERY_PARAM = 'SID';
Copy link
Contributor

Choose a reason for hiding this comment

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

Please explicitly specify visibility of this const.

/**
* Session ID in query param
*/
const SESSION_ID_QUERY_PARAM = 'SID';
Copy link
Contributor

Choose a reason for hiding this comment

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

Please explicitly specify visibility of this const. Also, now that SIDs and all their corresponding features are removed, I'm not sure if we should leave this param name as SESSION_ID_QUERY_PARAM with a SID value as that could be a bit misleading. In fact, the test that uses this only checks if a GET param is retained, so we can use any test param name.

@bgorski
Copy link
Contributor

bgorski commented Mar 7, 2021

@vlmed thank you for your contribution! In addition of a few small changes requested above, I noticed that a few tests ended with failures. That's probably not caused by the set of your PRs, but please double check. Either way, we will most likely have to wait until all tests except the semantic version checker end with successes before merging this.

@vlmed
Copy link
Contributor Author

vlmed commented Mar 8, 2021

@magento run all tests

@vlmed
Copy link
Contributor Author

vlmed commented Mar 8, 2021

@bgorski thank you for the review. I adjusted PR according to your recommendations. I will continue to follow the tests

@vlmed vlmed force-pushed the remove_deprecated_framework_interfaces branch from e9eefd0 to 4647684 Compare March 24, 2021 14:39
@vlmed
Copy link
Contributor Author

vlmed commented Mar 24, 2021

@magento run all tests

@bgorski
Copy link
Contributor

bgorski commented Mar 24, 2021

@magento run WebAPI Tests, Static Tests, Integration Tests, Functional Tests EE, Functional Tests CE, Functional Tests B2B

2 similar comments
@bgorski
Copy link
Contributor

bgorski commented Mar 27, 2021

@magento run WebAPI Tests, Static Tests, Integration Tests, Functional Tests EE, Functional Tests CE, Functional Tests B2B

@bgorski
Copy link
Contributor

bgorski commented Apr 2, 2021

@magento run WebAPI Tests, Static Tests, Integration Tests, Functional Tests EE, Functional Tests CE, Functional Tests B2B

@magento-automated-testing
Copy link

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.

3 similar comments
@magento-automated-testing
Copy link

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

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

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.

@Den4ik Den4ik self-assigned this Jun 1, 2021
@magento-automated-testing
Copy link

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.

1 similar comment
@magento-automated-testing
Copy link

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

Hi @bgorski, thank you for the review.
ENGCOM-9116 has been created to process this Pull Request
✳️ @bgorski, 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

@bgorski
Copy link
Contributor

bgorski commented Jun 8, 2021

There are some tests failing, but as for Integration tests, they are also failing for a clean build that includes all those products. As for functional tests and static tests, failures don't seem to be related to this set of PRs.

@bgorski bgorski added the Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests label Jun 8, 2021
@magento-automated-testing
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Pending Review
Development

Successfully merging this pull request may close these issues.

5 participants