Skip to content

Use php's strtolower to fix preparing attributes with unicode values #395

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

Closed
wants to merge 1 commit into from

Conversation

sibbl
Copy link

@sibbl sibbl commented Aug 3, 2017

See function getAttributeOptions inside Mage_ImportExport_Model_Import_Entity_Abstract. strtolower is used there, generating keys with wrong encoding for unicode values of attributes. They don't match the keys of the own (and absolutely correct!) strtolower used before. Thus, attributes will be set to null because i.e. straße (generated by this plugin's strtolower) doesn't match stra▒e (generated by Magento).

See function getAttributeOptions inside Mage_ImportExport_Model_Import_Entity_Abstract. strtolower is used there, generating keys with wrong encoding for unicode values of attributes. They don't match the strtolower version used here, which caused the attribute to be empty on import.
@sprankhub
Copy link
Collaborator

Thanks for your PR! We introduced AvS_FastSimpleImport_Helper_Data::strtolower, which uses mb_strtolower if it is available, in order to mitigate other encoding issues. I think that this is a good idea. You find more information if you search for strtolower in this repository, see e.g. #233 and #289.

Hence, I would absolutely stick to using the helper method. BUT you may be right that we missed another place where a simple strtolower is used. Then we should solve this issue. We already have a quite similar method. I am not sure where the referenced one Mage_ImportExport_Model_Import_Entity_Abstract::getAttributeOptions is used and when this leads to issues. Could you describe in detail how I can reproduce an issue during the import? Which attribute options should I create and which $data array should I import? What is the expected behaviour, what is the actual behaviour?

@sibbl
Copy link
Author

sibbl commented Aug 4, 2017

Thank for the detailed description! I'm unfortunately not familiar with all the importexport entities, that's why in my eyes it looked easier to change this as it's robust against changes in the Magento core code.

To reproduce, simply create a dropdown attribute with the attribute code "test", enable auto-creation for it, add it to an attribution set and then try to import any product with the attribution set and with test => 'maße'. It will autocreate the attribute option properly, but after the import, test will be imported with NULL (empty value) for this product as there's a special character in it.

@sprankhub
Copy link
Collaborator

Just to double check: Could you reproduce this issue with the latest dev branch? I am quite sure that it does not happen there... If you can reproduce it with the latest dev branch, I am happy to look into the issue.

@infabo
Copy link

infabo commented Oct 9, 2017

ImportExport uses PHP strtolower for "getAttributeOptions":

https://github.com/firegento/magento/blob/magento-1.9.3.6/app/code/core/Mage/ImportExport/Model/Import/Entity/Abstract.php#L377

To reproduce the issue, you need to enable the import of dropdown/multiselect attribute values and the attribute-value needs to contain an Umlaut character at position 0 (e.g. Übergang). strtolower is not able to lowercase "Übergang" to "übergang". It remains at "Übergang", whereas mb_strtolower is capable of and correctly lowers it to "übergang".

Edit:
Sry, you even don't need to have attribute value import enabled.

@pogster
Copy link
Contributor

pogster commented Mar 21, 2018

I encountered problems here as well. I had values with uppercase Ö. That does not work with strtolower.

On product import, \Mage_ImportExport_Model_Import_Entity_Product_Type_Abstract::_initAttributes uses \Mage_ImportExport_Model_Import_Entity_Abstract::getAttributeOptions. In my case, it would initialize the value as hÖlscher (strtolower core behaviour), and FastSimpleImport would on assignment look for hölscher (= not found).

@pogster
Copy link
Contributor

pogster commented Mar 22, 2018

Symptoms:
In dev mode the following notice:

Notice: Undefined index: hölscher in /var/www/html/app/code/community/AvS/FastSimpleImport/Model/Import/Entity/Product/Type/Simple.php on line 33

#0 /var/www/html/app/code/community/AvS/FastSimpleImport/Model/Import/Entity/Product/Type/Simple.php(33): mageCoreErrorHandler(8, 'Undefined index...', '/var/www/html/a...', 33, Array)
#1 /var/www/html/app/code/local/Gruenspar/Synchronization/Model/Import/Entity/Product.php(214): AvS_FastSimpleImport_Model_Import_Entity_Product_Type_Simple->prepareAttributesForSave(Array, false)

And option is not assigned correctly without error message.

I solved it by overwriting \Mage_ImportExport_Model_Import_Entity_Abstract::getAttributeOptions using the FastSimpleImport_Helper_Data strtolower.

Also, \AvS_FastSimpleImport_Model_Import_Entity_Product::isAttributeValid uses strotolower instead of the helper method as well, and needs to be changed or the option will then be declared as illegal. But that change is already included in the latest commits.

@sprankhub
Copy link
Collaborator

As already said, I think removing our custom strtolower method call is wrong. I implemented an alternative solution in #440. Do you mind reviewing and testing @pogster @infabo @sibbl?

@sprankhub sprankhub closed this Aug 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants