Improved performance of product category rewrites (disabled product & categories and product_use_categories) #3267
Conversation
github-actions
bot
added
Component: Adminhtml
| @@ -265,6 +265,16 @@ | |||
| <show_in_website>1</show_in_website> | |||
| <show_in_store>1</show_in_store> | |||
| </product_use_categories> | |||
| <create_url_for_disabled translate="label"> | |||
| <label>Generate URL rewrites for disabled categories and products</label> | |||
There was a problem hiding this comment.
this should go in a translation file too
fballiano
changed the title
|
Thanks for rebasing and submitting a new PR! However, I think it would be good to retain the original authorship of the commits. You can use |
|
I'll manually add the you and @MarcinNowakMacopedia in the commit message if that's ok |
|
Are you talking about adding "@" with teh github username to the commit message? It's not a big deal but I think that is not the best.. I end up getting emails over time when those commits are rebased and pushed even on other projects - also I think it might not update the Contributors section in the README automatically to add Marcin. But again, not blocker per-se. |
|
no I add the "coauthored by" message in the squash commit, I always do |
The only thing is that it doesn't prevent generating rewrites for child categories of a disabled category |
| @@ -974,6 +985,7 @@ protected function _saveRewriteHistory($rewriteData, $rewrite) | |||
| $rewriteData['options'] = 'RP'; // Redirect = Permanent | |||
| $this->getResource()->saveRewriteHistory($rewriteData); | |||
| } | |||
| $this->getResource()->clearProductRewrites($productId, $storeId); | |||
There was a problem hiding this comment.
the fact that this PR has so many undefined variables really makes me wonder.
is this call necessary here? should it be removed?
| @@ -375,7 +364,7 @@ protected function _refreshCategoryProductRewrites(Varien_Object $category) | |||
| $process = true; | |||
| $lastEntityId = 0; | |||
| $firstIteration = true; | |||
| while ($process == true) { | |||
| while ($process === true) { | |||
There was a problem hiding this comment.
the way the $process variable is handled... PHPStan complains that's actually a "while true"... which is not the best
|
this and mine #3251 |
|
this PR has too many problems that have to be addressed before merging, it's also a quite big one so I think it should go to a later release. @MarcinNowakMacopedia are you still interested in checking what's reported by phpstan? |
|
Most likely such PRs and those marked with "Don't Forget" will have to be promoted by a maintainer so that they can be corrected and approved by the others. |
|
Because of #3267 (comment) I'm closing this PR. |
This PR is a new version of #1405 since the original one was not editable by admin, was on the wrong branch and there was no feedback since a while.
@colinmollenhour I've also applied your PR macopedia#8 here in this code already.