set backend_type only for new created attributes - fix for issue #9219

[mhauri]

Description

At the moment all user defined attributes got set the backend_type based on frontend_input regardless if the attribute already exists and has a defined backend_type.

if (is_null($model->getIsUserDefined()) || $model->getIsUserDefined() != 0) {
    $data['backend_type'] = $model->getBackendTypeByInput($data['frontend_input']);
}

This PR fixes this by setting the backend_type only for new created attributes.

if (is_null($model->getBackendType())) {
    $data['backend_type'] = $model->getBackendTypeByInput($data['frontend_input']);
}

Fixed Issues (if relevant)

1. Custom Product Attribute changes 'backend_type' when 'is_user_defined = 1' and get updated/saved in Admin Backend #9219: Custom Product Attribute changes 'backend_type' when 'is_user_defined = 1' and get updated/saved in Admin Backend

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)

[vrann]

@mhauri with this PR the backend type will be assigned to the attributes which are created from the InstallData scripts (which are not not user-defined). Is there any reason to remove the condition is_null($model->getIsUserDefined()) ? Should you check both, user defined and backend type for being null?

[mhauri]

Well, actually it will only set the backend_type for attributes which haven't one.
So either the attribute is newly created in the Admin Backend, or it was missing in the install script and wasn't assigned during creation. But I'm fine with adding the is_null($model->getIsUserDefined()) condition or would $model->getIsUserDefined() != 0 be better?

[vrann]

@mhauri $model->getIsUserDefined() != 0 would be better because is user defined might be intentionally set to 0. Thank you.

[mhauri]

Here you go:

if (is_null($model->getBackendType()) && $model->getIsUserDefined() != 0) {
     $data['backend_type'] = $model->getBackendTypeByInput($data['frontend_input']);
}

[adragus-inviqa]

Is there a reason why a save of an attribute from UI is different from an $eavSetup->addAttribute() call? Why do you have to maintain two? Why are all the validations and sanitizations happening in that controller, and not in addAttribute(), also?

I never knew $validatorAttrCode = new \Zend_Validate_Regex(['pattern' => '/^[a-z\x{600}-\x{6FF} ][a-z\x{600}-\x{6FF}_0-9]{0,30}$/u']); was a thing. Is this available in addAttribute()? Why not? What happened with thin controllers?

You peeps are fixing a symptom here.

[mhauri]

@adragus-inviqa indeed, but maybe this should be done by another PR and not mixed with this one.

[okorshenko]

Hi @mhauri
Thank you for your contribution. Could you please verify failed tests. Some functionality has been broken by this fix. Please let us know if you would like to proceed with this PR or if we should close it. Thank you

[okorshenko]

Hi @mhauri
Could you please check my previous comment. Thank you

[okorshenko]

Closing this RP due to inactivity. @mhauri feel free to reopen this PR once ready. Thank you

[mhauri]

Hi @okorshenko sorry for late answer, I'm on it.

[okorshenko]

Hi @mhauri please reopen this PR once ready. Thank you

[mhauri]

Hi @okorshenko can you point me to which test fails exactly, because I was testing the core on a state before my changes (commit: 9224419) and there where already failing integration tests. So it would be glad If you can point me to which one is failing because of my commit.

[okorshenko]

Hi @mhauri
We have failed Functional Tests:
Please check build results: https://travis-ci.org/magento/magento2/jobs/252957886 and https://travis-ci.org/magento/magento2/jobs/252957887

[mhauri]

@okorshenko seems like the failing tests happened after your merge as the build did pass with my latest commit (https://travis-ci.org/magento/magento2/builds/222287766) but failed after your merge commit. As far as I can see is that also the current develop version build failed. (https://travis-ci.org/magento/magento2/builds/262152377)

[okorshenko]

Hi @mhauri
You are right. On your latest commit tests were green, but later we added Functional Tests to Travis. So I updated your branch with latest changes from develop and your branch got new tests for Travis. The tests were failing even on your latest commit but in our internal CI infrastructure. With the latest commits from develop branch, you can run these test on Travis.
I'm looking at latest results of functional tests on develop branch right now and checking what was wrong

UPD: latest build is green: https://travis-ci.org/magento/magento2/builds/262152377