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

Prevent empty string as useless product URL key on product imports #17563

Closed
wants to merge 2 commits into from

Conversation

jhruehl
Copy link
Contributor

@jhruehl jhruehl commented Aug 13, 2018

Using the given class for product import processes leads to comulative problems of handling product url key related processes.

Description

Since 2.4 at least the url key for the sku, which is given in an import file's row, is set to an empty string, if there is no corresponding url key or name column given. This leads to a variety of problems:

a) if no url key is given on import, it leads to changing the url key to Magento 2 standards by using the value in the name column. this totally ignores the fact that url keys get customized a lot of times and overwrites all efforts before. furthermore causing SEO problems, if as a consequence the url rewrites get adjusted accordingly. a product, which was exported to google shopping may not be reachable anymore by the link given to google or loose all linked SEO data by the next export given (if no 301 redirect is given).
b) if nor url key or name column is given on import, the url key is set as an empty string, which is even worse. this leads to several products having the same empty string as an url key, causing problems, when regenerating or reindexing url rewrites, because of duplicate entries. and so on and so on. furthermore an empty string as an url key is just useless.

Fixed Issue

if no url key or name column is given in the import file, this change prevents any url key to be set as or updated to an empty string, which is useless and causes even more problems in the aftermath.

Manual testing scenarios

  1. create an import file (csv) to update a product via sku and have no corresponding name or url key column in it
  2. check product url key before
  3. run import in backend or via an extension, which uses the given class
  4. check product url key again

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 on Travis CI are green)

Using the given class for product import processes leads to comulative problems of handling product url key related processes.

Since 2.4 at least the url key for the sku, which is given in an import file's row, is set to an empty string, if there is no corresponding url key or name column given. This leads to a variety of problems:

a) if no url key is given on import, it leads to changing the url key to Magento 2 standards by using the value in the name column. this totally ignores the fact that url keys get customized a lot of times and overwrites all efforts before. furthermore causing SEO problems, if as a consequence the url rewrites get adjusted accordingly. a product, which was exported to google shopping may not be reachable anymore by the link given to google or loose all linked SEO data by the next export given (if no 301 redirect is given).
b) if nor url key or name column is given on import, the url key is set as an empty string, which is even worse. this leads to several products having the same empty string as an url key, causing problems, when regenerating or reindexing url rewrites, because of duplicate entries. and so on and so on. furthermore an empty string as an url key is just useless.

synopsis: if no url key or name column is given in the import file, this change prevents any url key to be set as or updated to an empty string, which is useless and causes even more problems in the aftermath.
@magento-engcom-team
Copy link
Contributor

Hi @jhruehl. Thank you for your contribution
Here is 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-engcom-team give me test instance - deploy test instance based on PR changes
  • @magento-engcom-team give me {$VERSION} instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

@ihor-sviziev
Copy link
Contributor

@dmanners could you review this PR?

@ihor-sviziev ihor-sviziev requested a review from dmanners August 14, 2018 13:43
@magento-engcom-team
Copy link
Contributor

@jhruehl thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

@dmanners
Copy link
Contributor

dmanners commented Sep 5, 2018

Hi @jhruehl thanks for this pull request. Let me take a look at it today and get back to you on this.

@dmanners
Copy link
Contributor

Hi @jhruehl we have run the following cases:

Case 1:

  • Create simple product.
  • Create an import file (csv) to update a product via sku. Csv file doesn't contain columns "name" and "url_key" and their appropriate values.
  • Check product url key before
  • Run import in backend
  • Check product url key again

Result: Product url key is unchanged 👍

Case 2:

  • Create simple product.
  • Create an import file (csv) to update a product via sku. Csv file doesn't contain column "url_key",but contain column "name" with empty value.
  • Check product url key before
  • Run import in backend
  • Check product url key again

Result: Product url key is changed and contain Product Name value

Case 3:

  • Create simple product.
  • Create an import file (csv) to update a product via sku. Csv file doesn't contain column "url_key",but contain column "name" with some data.
  • Check product url key before
  • Run import in backend
  • Check product url key again

Result: Product url key is changed and contain Product Name value.

What do you think of the results for case 2 & 3? For me if the url_key column is not in the csv I would expect the value to not be updated. But I am happy to hear your thoughts on this.

@jhruehl
Copy link
Contributor Author

jhruehl commented Sep 19, 2018

@dmanners I agree. If there is no url_key colum present, the url key should no be updated. It's as easy as that.

Though if the product is newly created via the import and there is no url_key column present, the url key should probably be created by using the name column. BUT with the standard functionality of creating URL Keys, so that existing plugins and/or observers to customize the url key generation process can kick in.

If none of those columns are present, no url key should be added or at least something unique and not an empty string. Overwriting existing url keys with empty strings.. wow.. this stupidity still bedazzles me.

I don't know, but in all our supervised stores CSVs are used in daily and hourly crons to update stocks, special prices and so on.
And we have customized url keys in our stores with an entity type based prefix followed by the sku and the name. In the current implementation we would have to export those to the ERP so that the ERP can put them into every CSVs created for product imports. Who doesn't like redundancies and waste of resources?
Of course we wouldn't allow that to happen and patch the core class responsible for the import anyway.

Everything else will probably result in nightmares for the responsible SEO folks and as a consequence we, the developers, have to clean the mess up. Repairing url keys, url rewrites and creating 301 redirects where still possible. I can't remember the last time I had so much fun. ;)

@jhruehl
Copy link
Contributor Author

jhruehl commented Sep 19, 2018

Anyway. My current pull request doesn't change anything. If a url key can be generated, it still will be set.
My change just prevents empty strings as url keys on imports, nothing more, nothing less. So where is the problem with the merge?

@hostep
Copy link
Contributor

hostep commented Sep 19, 2018

It looks like there is a competing PR which tries to solve the same issue I think: #17882

Not sure which one of the two is the better one

@jhruehl: I feel your pain, have been there many many times, url_key and url rewrite problems in Magento are really not fun to deal with and do still happen way too many times.

@jhruehl
Copy link
Contributor Author

jhruehl commented Sep 20, 2018

@hostep Yeah. It's the same issue, though the guy's solution seems kinda overcoded.

@dmanners
Copy link
Contributor

Ah cool thanks for flagging this @hostep I will talk with @nuzil who is reviewing the other PR and see what the best option to progress here would be.

Sorry for any confusion or duplicated work.

@nuzil
Copy link
Contributor

nuzil commented Sep 21, 2018

Hi @jhruehl I just reviewing similar PR. I will check the both implementations one more time and will come back to you :-)

@nuzil
Copy link
Contributor

nuzil commented Oct 4, 2018

Thanks a lot for your contribution. I'm closing this PR cause other was already merged
#17882 (comment)

@nuzil nuzil closed this Oct 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants