-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
base: 2.5-develop
Are you sure you want to change the base?
Remove deprecated framework interfaces #32328
Conversation
Hi @vlmed. Thank you for your contribution
❗ Automated tests can be triggered manually with an appropriate comment:
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. 🕙 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 run all tests |
Hello @coderimus, @sivaschenko
Please check and help choose the right way to solve them |
/** | ||
* The key arguments in the data component | ||
*/ | ||
const COMPONENT_ARGUMENTS_KEY = 'arguments'; |
There was a problem hiding this comment.
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'; |
There was a problem hiding this comment.
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'; |
There was a problem hiding this comment.
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'; |
There was a problem hiding this comment.
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.
@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. |
@magento run all tests |
@bgorski thank you for the review. I adjusted PR according to your recommendations. I will continue to follow the tests |
e9eefd0
to
4647684
Compare
@magento run all tests |
@magento run WebAPI Tests, Static Tests, Integration Tests, Functional Tests EE, Functional Tests CE, Functional Tests B2B |
2 similar comments
@magento run WebAPI Tests, Static Tests, Integration Tests, Functional Tests EE, Functional Tests CE, Functional Tests B2B |
@magento run WebAPI Tests, Static Tests, Integration Tests, Functional Tests EE, Functional Tests CE, Functional Tests B2B |
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
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. |
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. |
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. |
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
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. |
Hi @bgorski, thank you for the review.
|
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. |
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. |
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
.According to the dependencies of
lib/internal/Magento/Framework/App/TemplateTypesInterface.php
and comment I moved this interface toMagento\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 methodsgetMaxWidth()
andgetMaxHeight()
and unused deprecated contants from this class.- All dependencies have been fixed.
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 classMagento\Framework\Module\Manage
.- I changed the behaviour of the deprecated method
Magento\Framework\Module\Manage::isOutputEnabled
. I removed the code belonging to the classMagento\Framework\Module\Output\Config
from theisOutputEnabled
method.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 classMagento\Framework\View\Element\UiComponentFactory
.- Some constants from
ManagerInterface
are using in the classMagento\Framework\View\Element\UiComponentFactory
. So i moved them to the classUiComponentFactory
- All dependencies has been fixed;
-
9. lib/internal/Magento/Framework/Filesystem/ExtendedDriverInterface.php
-
ExtendedDriverInterface
- I tried to delete it and move thegetMetadata()
method toDriverInterface
.- But the classes implementing
DriverInterface
were broken. Because this requires the implementation of thegetMetadata()
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)
Questions or comments
Contribution checklist (*)