URL rewrites not being generated - calls to doReplace() actually insert instead.

simonworkhouse commented 6 years ago

We have been experiencing an intermittent issue where entries into the url_rewrite table are not being generated automatically. I have noticed that this issue has been mentioned in the past but I believe that we have isolated the cause.

Magento\UrlRewrite\Model\Storage\DbStorage::doReplace(...) doesn't do a REPLACE but an INSERT, and errors for duplicate entries aren't throwing an AlreadyExistsException as expected.

A general gist of the code path is as follows: Magento\CatalogUrlRewrite\Model\UrlRewriteBunchReplacer::doBunchReplace(...) -> Magento\UrlRewrite\Model\Storage\DbStorage::doReplace(...) -> Magento\Framework\DB\Adapter\Pdo\Mysql::insertMultiple(...) -> Magento\Framework\DB\Adapter\Pdo\Mysql::insertArray(...)

When we updated the doReplace(...) and insertMultiple(...) functions to pass the replace strategy through to insertArray(...) everything worked as expected. Would I be correct in assuming that this was the intended strategy and that is was just missing from doReplace and insertMultiple? Why would it not be a replace?

Also, any calls to Magento\Framework\DB\Adapter\Pdo\Mysql::insertMultiple(...) will fail if there are duplicates in the data based on unique constraints. This function doesn't take this into account and doesn't have the ability to pass the strategy through to insertArray(...). There also doesn't appear to be an equivalent replaceMultiple(...).

I would guess that this might be causing some other bugs we have noticed, particularly with importing catalog data from a CSV.

Preconditions

Magento 2.2.5
PHP 7.1.20
MySQL 5.7.23
Multiple stores set up and enabled with the same products and root category

Steps to reproduce

Assuming that the category data will fail for a duplicate entry:

Update the URL key for a category

Actual result

Received the following MySQL error running the query that Magento genereted: Error Code: 1062. Duplicate entry 'some-product-1' for key 'URL_REWRITE_REQUEST_PATH_STORE_ID'

An AlreadyExistsException didn't appear to have been thrown, or was silently handled.

Expected result

The correct entries in the url_rewrite table are created.

magento-engcom-team commented 6 years ago

Hi @simonworkhouse. Thank you for your report. To help us process this issue please make sure that you provided the following information:

[ ] Summary of the issue
[ ] Information on your environment
[ ] Steps to reproduce
[ ] Expected and actual results

Please make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce. To deploy vanilla Magento instance on our environment, please, add a comment to the issue:

@magento-engcom-team give me {$VERSION} instance

where {$VERSION} is version tags (starting from 2.2.0+) or develop branches (2.2-develop +). For more details, please, review the Magento Contributor Assistant documentation.

@simonworkhouse do you confirm that you was able to reproduce the issue on vanilla Magento instance following steps to reproduce?

[ ] yes
[ ] no

ghost commented 6 years ago

@simonworkhouse, thank you for your report. We were not able to reproduce this issue by following the steps you provided. Please provide more detailed steps to reproduce or try to reproduce this issue on a clean installation or latest release.

simonworkhouse commented 6 years ago

Has a human actually read the bug report? As I stated, this is an intermittent issue that happens when the catalog data is under certain conditions. Unfortunately I cannot simply provide an SQL dump for obvious reasons (privacy issues, etc...) and it could take quite some time for me to generate a CSV (or series of CSVs) that one could use to import into a clean installation that simulates these conditions.

This insistence on everything needing to be immediately reproducible from a clean installation is nonsense, particularly when the details of the issue can be clearly seen if one just reads the bug report and reviews the code.

Here are a number of bug reports related to this issue: https://github.com/magento/magento2/issues/6606 https://github.com/magento/magento2/issues/8227 https://github.com/magento/magento2/issues/12662 https://github.com/magento/magento2/issues/5863

Clearly there's a serious issue with the way that the URL rewrites are generated and updating the Magento\UrlRewrite\Model\Storage\DbStorage::doReplace(...) function to actually do a REPLACE fixes the issue in our instance. Also, the issues with Magento\Framework\DB\Adapter\Pdo\Mysql::insertMultiple(...) need to be noted and someone needs to manually review this.

ghost commented 6 years ago

@simonworkhouse, we believe that this issue exists. But we cannot add issue to our work backlog and fix it until we have obvious and 100% clear way to reproduce it.

orlangur commented 6 years ago

@simonworkhouse,

Magento\UrlRewrite\Model\Storage\DbStorage::doReplace(...) doesn't do a REPLACE but an INSERT

There is nothing wrong with that, method names should not be mixed up with underlying SQL.

Please provide an isolated test case which reproduces a problem on a clean Magento installation.

simonworkhouse commented 6 years ago

@orlangur

There is nothing wrong with that, method names should not be mixed up with underlying SQL.

I do realise that, but the problem is due to the fact it is actually trying to perform the action of a replace with an insert. There doesn't appear to be any protection to prevent duplicates in the batch update either, so batches will fail if there are ever any duplicates. Additionally, the exception that is supposed to be thrown on duplicates doesn't ever appear to be thrown. MySQL logs indicate that the transaction was rolled back so could it be possible that the exception wasn't thrown due to this?

The query that Magento was trying to run generated the MySQL error "Error Code: 1062. Duplicate entry 'some-product-1' for key 'URL_REWRITE_REQUEST_PATH_STORE_ID'" and updating the code to perform a REPLACE fixed the issue for me. Whether or not that's a byproduct of another bug/issue I can't be sure without digging further, but from reviewing the code I would suggest that it was probably intended to perform a replace initially.

I will have to spend a bit of time setting up a test case for you, as I can't be certain if it has anything to do with a multi store setup, CSV imports, the category structure, something else or a combination of these. You may have to wait a little while for that as I don't have too much time available at the moment. However; I would have thought that reviewing the code would highlight the issue clearly enough. Is there any reason as to why it's not performing a REPLACE?

orlangur commented 6 years ago

@simonworkhouse because old URL rewrites are removed prior to inserting:

protected function doReplace(array $urls)
    {
        $this->deleteOldUrls($urls);

        $data = [];
        foreach ($urls as $url) {
            $data[] = $url->toArray();
        }
        try {
            $this->insertMultiple($data);

Feel free to reopen issue whenever you find out exact steps.

simonworkhouse commented 6 years ago

@orlangur

because old URL rewrites are removed prior to inserting:

Yes, I had noticed that. However; it's not necessarily the conflict with the existing URLs that is the issue, it appears that it might be the data that it's trying to insert that contains the duplicates.

Are we able to have this issue re-opened please?

orlangur commented 6 years ago

might be the data that it's trying to insert that contains the duplicates

I believe it's not possible by construction. You can play with your buggy instance to find out if this is the case. To me it looks like you have some orphan records due to improper migration or some other sort of URL rewrite creation and thus they stay in database after deleteOldUrls.

simonworkhouse commented 6 years ago

@orlangur Well it certainly is an issue and isn't limited to our instance, just see the other bug reports. It may not be related to the data going in, it might be the call to $this->deleteOldUrls($urls); not working correctly or possibly something else.

Like I said, I was only able to get so far with debugging, located the query that was failing and reported a general gist of the code path. I can assure you that this is definitely an issue, I'm just not certain on the conditions that are required to cause it.

I will allocate some time to this, and try to determine the root cause. Please re-open the issue and I will see about providing the data over the weekend.

simonworkhouse commented 6 years ago

@orlangur I have been able to determine the cause of the issues that we were experiencing, but updating the query to a REPLACE will not resolve the core issues with the way that the data in the url_rewrite table is managed. It is my opinion that the whole rewrite system is in need of a bit of an overhaul and needs to be thoroughly tested.

Changing the configuration values of "catalog/seo/product_url_suffix", "catalog/seo/category_url_suffix" and "catalog/seo/product_use_categories" can cause serious issues with the URL rewrites and there doesn't appear to be any mechanism to regenerate them.

I will report each variation of this issue as a separate bug and also the other bugs that I encountered during testing.

simonworkhouse commented 6 years ago

@orlangur See https://github.com/magento/magento2/issues/17586 for the outline of a number of scenarios where the URL rewrite generation has issues.

ghost commented 6 years ago

@simonworkhouse

Is this a Magento 2 instance which is migrated from magento 1? Because if it is a Migrated magento 2 it all has to do with the data migration process.

100% sure this is breaking the magento 2 database in different ways. I have migrated my store 6 times and this issue was coming back every time. There are a lot of issues after data migration, this is the biggest one. They need to rewrite this tool and Developers of other tools should be warned this issue occur in the data migration process.

So if you install a clean magento 2 and import your products without migrating anything else you will see no problems.

Best Regards.

eWolf62095 commented 6 years ago

Sires... Do we have any temporary fix for this or band-aid fix? Any workarounds will do.

ghost commented 6 years ago

@eWolf62095 @thlassche

What version of magento are you using? Did you migrate data? or did you export import your data. Like products, categories, attributes and attribute sets?

If you migrated your data from a magento 1 instance this is not good but normal behavior in magento 2. The database gest broken in several ways, one of them is URL rewrites the next one is unable to change Design configurations. There are more bot lest stay on topic.

If these issues occur after export and importing data (not true data migration tool) on a newly fresh Magento 2 instance of the latest version this should be addressed on the other forums.

The problem should not be fixed after data migration, no the data migration tool should be checked, repaired or much better rewritten.

For the record: At the moment there is only a payed solution for the mass and the non programmer or SQL expert. Yes i am against this open source payed solutions but now I started promoting this because there is nothing to expect from Magento itself to resolve these issues quickly. Contact @thlassche If you wish, he sells you the solution if you wish.

Please feel free to answer my questions

So again: A. Did you migrate data? Yes? Which tool did you use? B.Or did you export and import your data. Like products, categories, attributes and attribute sets? C. None of these Things, clean Magento 2.

Best regards.

ghost commented 6 years ago

Best,

Thank you very much for the information.

I am going to try to get this patch.

Do you have information about the patch, a patch number and file name?

Best regards Bram Slagboom

Adres/Address

Koopjesboom V.O.F.

Lorentzweg 26

2964LN Groot-Ammers

Tel/Phone : 0031184634329

Mobile : 0031651180371

E-Mail : info@koopjesboom.com

https://kbmodelcars.com/ https://kbmodelcars.com

https://ownstock.eu/ https://ownstock.eu

KVK : 58220259

Van: eWolf62095 notifications@github.com Verzonden: woensdag 24 oktober 2018 11:36 Aan: magento/magento2 magento2@noreply.github.com CC: koopjesboom info@koopjesboom.com; Mention mention@noreply.github.com Onderwerp: Re: [magento/magento2] URL rewrites not being generated - calls to doReplace() actually insert instead. (#17378)

@koopjesboom https://github.com/koopjesboom hi sire, just for an update. Magento Support provided a patch for us. The patch was about "URL Key not updated during product import". The issue is different from what's in this thread however the patch provided also fixed the issue that I experienced same on this thread. Sadly I cannot share you the patch because it is the owned by our company and only our infra team has accessed on the support desk of Magento 2.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/magento/magento2/issues/17378#issuecomment-432586092 , or mute the thread https://github.com/notifications/unsubscribe-auth/AOippDHfpxWYTAsmGP0B-Zc5ps8iaKD_ks5uoDR7gaJpZM4VvxTE . https://github.com/notifications/beacon/AOippG3scUJKU9Zx4YDKPk82V8o94Q3Mks5uoDR7gaJpZM4VvxTE.gif

eWolf62095 commented 5 years ago

Hi, sir sorry for the late response, You can refer to this name for the patch: MDVA-7285_EE_2.2.4_v1.composer.patch Thank you @koopjesboom

PascalBrouwers commented 3 years ago

Came here for the same issue, problem is when you import a product with a sku that is not lowercase. That product will get a new product_id and the whole $this->deleteOldUrls($urls); does not work then and you get the error.

magento / magento2