Skip to content

Do not overwrite URL Key with blank value #17882

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.

Sign up for GitHub

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

Merged
magento-engcom-team merged 12 commits into from
Oct 4, 2018
Merged

Do not overwrite URL Key with blank value #17882

Changes from 2 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
a021d5f
Do not overwrite URL Key with blank value
josephmcdermott Aug 31, 2018
a02bf03
Removed `@see` references from comments
josephmcdermott Sep 3, 2018
e48683c
Reverted return type of `getUrlKey` back to always be string
josephmcdermott Sep 3, 2018
7df4e12
Improved logic of `getUrlKey` to be more specific around the name pro…
josephmcdermott Sep 3, 2018
fffb6ff
Simplified logic of `isNeedToValidateUrlKey` to trust logic from `get…
josephmcdermott Sep 3, 2018
002aa19
Updated expected url_key values of integration test testImportWithout…
josephmcdermott Sep 13, 2018
172e2ec
Enabled @magentoDbIsolation on testImportWithoutUrlKeys() due to `sim…
josephmcdermott Sep 13, 2018
3a54946
Delete test product `simple3` before importing the CSV to avoid conta…
josephmcdermott Sep 19, 2018
2fb7183
Force reload of products during test to ensure they are not coming fr…
josephmcdermott Sep 19, 2018
fda664a
Updated testImportWithoutUrlKeys() to create new simple in CSV for th…
josephmcdermott Sep 19, 2018
abd95bd
Reverted previous changes which are no longer required
josephmcdermott Sep 19, 2018
0f26b52
Trying last test run with @magentoDbIsolation as now other (completel…
josephmcdermott Sep 19, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 23 additions & 3 deletions app/code/Magento/CatalogImportExport/Model/Import/Product.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1561,7 +1561,9 @@ protected function _saveProducts()
}
$rowScope = $this->getRowScope($rowData);

$rowData[self::URL_KEY] = $this->getUrlKey($rowData);
if ($urlKey = $this->getUrlKey($rowData)) {

@nuzil nuzil Sep 3, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not recommend here to change return type of the method (line 2725, 2739). As user of getUrlKey method I expecting to receive back some url. Can you please change this implementation to check if URL is empty then do not overwrite it.

$rowData[self::URL_KEY] = $urlKey;
}

$rowSku = $rowData[self::COL_SKU];

Expand Down Expand Up @@ -2406,6 +2408,16 @@ public function validateRow(array $rowData, $rowNum)
*/
private function isNeedToValidateUrlKey($rowData)
{
/**
* If the product exists, assume it already has a URL Key and even
* if a name is provided in the import data, it should not be used
* to overwrite that existing URL Key the product already has.
*/
$isSkuExist = $this->isSkuExist($rowData[self::COL_SKU]);
if ($isSkuExist && !array_key_exists(self::URL_KEY, $rowData)) {
return false;
}

return (!empty($rowData[self::URL_KEY]) || !empty($rowData[self::COL_NAME]))
&& (empty($rowData[self::COL_VISIBILITY])
|| $rowData[self::COL_VISIBILITY]
Expand Down Expand Up @@ -2710,20 +2722,28 @@ protected function getProductUrlSuffix($storeId = null)

/**
* @param array $rowData
* @return string
* @return string|bool

@nuzil nuzil Sep 3, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comment before about return type

@josephmcdermott josephmcdermott Sep 3, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that's a good point, I was on the fence myself about this change.

* @since 100.0.3
*/
protected function getUrlKey($rowData)
{
if (!empty($rowData[self::URL_KEY])) {
return strtolower($rowData[self::URL_KEY]);
}

/**
* If the product already exists, do not overwrite its
* URL Key with a value generated from the provided name.
*/
if ($this->isSkuExist($rowData[self::COL_SKU])) {
return false;

@nuzil nuzil Sep 3, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comment before about return type

}

if (!empty($rowData[self::COL_NAME])) {
return $this->productUrl->formatUrlKey($rowData[self::COL_NAME]);
}

return '';
return false;
}

/**
Expand Down