Don't set a source model on the attribute when it's not needed. This …

[hostep]

…avoids accidentally persisting the source model to the database when using multiselect attributes.

Description

This removes calling a setter (setSourceModel) inside a getter (getSource ) which is considered bad practice, since it changes state of an object, and a getter shouldn't change the state of an object.

This is not some theoretical case, please see the manual testing scenario or #13156 to have an example where this can be seen in practice.

Fixed Issues (if relevant)

1. Updating attribute option data through API will set unwanted source_model on the attribute #13156: Updating attribute option data through API will set unwanted source_model on the attribute

Manual testing scenarios

1. Setup a clean Magento installation
2. Create an Integration with full access rights & activate it, I'm assuming the access token is abcd (see step 6)
3. Using the adminhtml, create a new multiselect product attribute, use attribute code: test_multi
4. Take a look in the database, in the table: eav_attribute, notice that the source_model is NULL
5. Using the adminhtml, add some options to the attribute and verify that the source_model is still NULL in the database
6. Now add an option through a REST call (make sure to use a new option):

curl -k -X POST "https://domain.tld/rest/all/V1/products/attributes/test_multi/options" -H "accept: application/json" -H "Content-Type: application/json" -H "Authorization: Bearer abcd" -d "{ \"option\": { \"label\": \"whatever\" }}"

7. Take a look at the database again, expected is that source_model stays NULL, but it doesn't, it changes to Magento\Eav\Model\Entity\Attribute\Source\Table

This PR fixes this.

Further

The reason why this only happens with multiselect attributes and not with select attributes, is because of this piece of code: https://github.com/magento/magento2/blob/f36db11/app/code/Magento/Catalog/Model/ResourceModel/Eav/Attribute.php#L383-L392
I don't know if multiselect should be added as well next to the condition on the select in that piece of code. I didn't try to figure it out anymore, since it was already complex enough to figure out all what was going on :)

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)

[magento-engcom-team]

Hi @hostep. 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

[hostep]

Regarding the static test failures, it asks me to write a whole bunch of documentation for methods which I didn't touch and have very little knowledge about. Should I skip these and let someone else who authored this code handle them?
Also: what's the point for writing documentation on getters and setters, doesn't the method name already explain exactly what it does?

Thanks! :)

[magento-engcom-team]

Hi @hostep. Thank you for your contribution.
We will aim to release these changes as part of 2.3.1.
Please check the release notes for final confirmation.