addAttribute mapping and updateAttribute confusions

[wojtekn]

\Magento\Eav\Setup\EavSetup::addAttribute() method uses property mapping but \Magento\Eav\Setup\EavSetup::updateAttribute() doesn't. This mapping is done in classes:

• \Magento\Eav\Model\Entity\Setup\PropertyMapper
• \Magento\Catalog\Model\Resource\Setup\PropertyMapper

What is the point to copy this confusing feature from Magento 1 to Magento 2? Wouldn't it make more sense to use the same key names in both cases?

Preconditions

1. Magento 2 source code.

Steps to reproduce

1. Open [Magento root dir]/app/code/Magento/Eav/Setup/EavSetup.php
2. Observe EavSetup::addAttribute method declaration.
3. Observe EavSetup::updatelAttribute method declaration.

Expected result

EavSetup::addAttribute and EavSetup::updatelAttribute signatures should be consistent (both accepting mapped properties or not mapped properties).

Actual result

EavSetup::addAttribute and EavSetup::updatelAttribute signatures are inconsistent.

[daim2k5]

The GitHub issue tracker is intended for technical issues only. Please refer to the Community Forums or Magento Stack Exchange site for technical questions. In your case, the programming questions forum is likely the most appropriate. Feel free to reopen this issue if you think you have encountered a bug in Magento 2.

[wojtekn]

@daim2k5 this is technical issue, not a question.

[daim2k5]

@wojtekn ok, but you ask only two question. I still don't see a technical issue. Maybe you can describe it with more details?

[wojtekn]

@daim2k5 issue is about inconsistency in M2 code, so I wouldn't consider this as question but technical issue.

IMO it's confusing that you need to use two different key variants in install and upgrade scripts.

[veloraven]

@wojtekn is this issue still actual?
If it is please describe detailed steps, actual result and expected result.
If it is not, please close it.

[wojtekn]

@veloraven it's still not fixed, both methods accept different sets of arguments.

Expected result is to have both methods accepting mapped properties or to have both methods accepting not mapped properties. It is confusing that "add" and "update" methods accept arguments in two different format.

Assuming that currently there may exist huge amount of code relying on this (community extensions and other custom code), it would make sense to modify updateAttribute method to accept both mapped and not mapped properties.

[rparsi-commer]

@wojtekn @daim2k5 @veloraven
It IS an issue just as wojtekn describes; the inconsistency is confusing and has caused me some pain in writing my custom module.

@daim2k5 @veloraven
Can you point me to actual documentation that explicitly states what addAttribute expects in the array parameter (what keys are allowed/expected)?
I'm referring to the $attr argument.
Class namespace is Magento\Eav\Setup\EavSetup
Method signature is addAttribute($entityTypeId, $code, array $attr)

[magento-engcom-team]

@wojtekn, thank you for your report.
We've created internal ticket(s) MAGETWO-84612 to track progress on the issue.

[Vedrillan]

I'll add that the field map translation done in the addAttribute method is also doing more than just mapping.

Mappers used such as Magento\Eav\Model\Entity\Setup\PropertyMapper are setting default values for missing fields, which means that if you try to update only a single field of an attribute with the addAttribute method you will end up with all other mapped attributes reset to their default values.

For instance if you try to do the following:

$this->eavSetupFactory->create(['setup' => $setup])
    ->addAttribute(
        \Magento\Catalog\Model\Product::ENTITY,
        'visibility',
        ['label' => 'Where it can be seen']
    );

The result will be the visibility attribute having the new custom label but being set from select to varchar type as well because of the mapper default values.

In the end the attribute CRUD from the EavSetup class should be refactored to be indeed a bit more dev friendly.

[magento-engcom-team]

Hi @wojtekn thank you for your report. The method updateAttribute should receive already mapped data. We do understand that this might confuse you, but this is responsibility of client code to prepare the data for update