-
Notifications
You must be signed in to change notification settings - Fork 9.4k
#29174 Add save_rewrites_history to categories via API #29205
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
#29174 Add save_rewrites_history to categories via API #29205
Conversation
Hi @amenk. 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 |
* Support save_rewrites_history in save
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.
Hi @amenk!
Thank you for your contribution!
Could you please check my comments?
Could you, also, please add coverage with webapi-functional tests?
Thank you!
app/code/Magento/CatalogUrlRewrite/Plugin/Webapi/Controller/Rest/InputParamsResolver.php
Outdated
Show resolved
Hide resolved
@@ -82,6 +82,10 @@ public function save(\Magento\Catalog\Api\Data\CategoryInterface $category) | |||
$existingData = array_diff_key($existingData, array_flip(['path', 'level', 'parent_id'])); | |||
$existingData['store_id'] = $storeId; | |||
|
|||
if ($category->getData('save_rewrites_history', null) !== null) { |
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.
It should be placed in Magento_CatalogUrlRewrite, not in Magento_Catalog.
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.
Do you have a clue how we could do that?
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.
In the first place my code is very similar to the solution products, just the categories are persisted differently and I am wondering which is the better way :)
I.e. if we change it for categories, we might need to adapt products as well.
I see several options:
- Mess with \Magento\Catalog\Model\CategoryRepository::getExtensibleDataObjectConverter (which is already deprecated)
- Intercept a different method, in the before save event, and somehow get the info that rewrites should be created from the request
- .... ?
Help is very much appreciated :)
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.
The problem that approach is lead to violation of modularity.
I see that for save from Adminhtml area, it uses observer to set a value and ResourceModel to save:
observer name="category_save_rewrites_history_setter"
Could you play somehow with \Magento\Catalog\Model\CategoryRepository::getExtensibleDataObjectConverter
? (the method is deprecated because class should be injected into the constructor parameters).
I see that it can handle custom attributes: \Magento\Framework\Api\ExtensibleDataObjectConverter::processCustomAttributes
Could we check why it does not help to solve our issue?
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.
The problem that approach is lead to violation of modularity.
absolutely
I see that for save from Adminhtml area, it uses observer to set a value and ResourceModel to save:
observer name="category_save_rewrites_history_setter"
yes, but no access to what was sent via the API, here
Could you play somehow with \Magento\Catalog\Model\CategoryRepository::getExtensibleDataObjectConverter ? (the
method is deprecated because class should be injected into the constructor parameters).
I see that it can handle custom attributes:
\Magento\Framework\Api\ExtensibleDataObjectConverter::processCustomAttributes
Could we check why it does not help to solve our issue?
Maybe ... I am still wondering if that flag should be a custom attribute at all
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.
Oh, I really would like to avoid such complications.
In such a case, it will be nice to have a mechanism for extension of Custom Attributes with not only EAV attributes but also with some configuration (more XML!).
But we already have Extension Attributes, which, could be used for this.
I prefer to look into two possible ways:
- A short one - look into how we are saving category.
Probably, we need to just copy 'all other' fields from category data as it works for the product:
$productDataArray = $this->extensibleDataObjectConverter - A long one but more solid - look if we can replace usage of a custom attribute for extension attributes.
But it still is required to think about possible BC breaks (at least for products, where it works previously).
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.
Thanks for your feedback @swnsma - I choose the blue pill (1.) for now. Let's see what the full test suite say.
I might still be BC breaking in a way, that some module does not expect all data to be copied in the repository's save, but it is very unlikely.
Actually I am not sure how repositories are supposed to work: Whether they should the attributes to the ones clearly specified (like EAV attributes) or not.
Option 2. would be great to implement, because it also saves us from all the hassle in InputParamsResolver, but I just don't have the resources to do that now -- at least we would have a integration test now if this get's merged :)
In the end of the day, I am not even sure if this setData() approach is nice to trigger the URL key regeneration - maybe somebody has even some better idea here.
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.
I also do not really like the idea to pass all data with setData()
. It also a hack but, at least, it does not provide modularity issue 😅
But, in general, it will be nice to ask someone else for additional ideas.
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.
I would prefer option 1 if it is enough to solve the issue.
Looks like the safest and the easiest way.
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.
That is like it is currently implemented in the PR... Can you take over and fix the tests? Thanks in advance
I thought about doing that, yes :) Do you have some suggestion which test would be a good starting point. |
I think I will use \Magento\Catalog\Api\CategoryRepositoryTest::testUpdate as a base test |
@swnsma functional test added, no we just have to solve the architectural problem in Feel free to adapt the pull request, if you have an idea how to fix it and now that a proper test is in place. |
Actually there are tons of modularity problems between Catalog and CatalogUrlRewrite --- save_rewrites_history is used in Magento_Catalog and url_key of course as well ... The simples possible fix for the full issue would be
We could do something similar for products and get rid of the full file app/code/Magento/CatalogUrlRewrite/Plugin/Webapi/Controller/Rest/InputParamsResolver.php |
…4-save-rewrites-for-categories # Conflicts: # dev/tests/api-functional/testsuite/Magento/Catalog/Api/CategoryRepositoryTest.php
@magento run all tests |
@magento run all tests |
@swnsma whom? |
@magento run all tests |
Failed functional tests not related to the changes in this PR |
Hi @swnsma, thank you for the review.
|
@magento run Functional Tests EE, Functional Tests B2B |
✔️ QA Passed Manual testing scenario
BEFORE applying changes provided in this PR, the old url_rewriew record has been removed, and only the new record existed AFTER switching to the PR, the previous record is not removed |
@magento create issue |
Hi @amenk, thank you for your contribution! |
Support save_rewrites_history to categories API
Description (*)
See #29174
Fixed Issues (if relevant)
Manual testing scenarios (*)
See example in #29174
Contribution checklist (*)
Resolved issues: