Feature: EAV attribute setup configuration builder

[nevvermind]

/cc #4266

This PR introduces an OOP way of adding EAV attributes, by using attribute configuration builders.
My main concerns with this were:

• expressiveness and explicitness (the opposite of terse array keys)
• as strict as possible, but no more than that
• fail fast and early
• self-documented
• the usual OOP perks (IDE auto-completion, hiding complexity, normalising, type hinting, easier refactoring etc.)

Only product, category, customer and customer_address types are supported.
Class design looks like this:

AttributeConfiguration (abstract)
├── CatalogAttributeConfiguration (abstract)
│   ├── CategoryAttributeConfiguration
│   └── ProductAttributeConfiguration
└── GenericCustomerAttributeConfiguration (abstract)
    ├── CustomerAddressAttributeConfiguration
    └── CustomerAttributeConfiguration

All classes introduced here are builders, which get converted into the EAV setup array you all love, nothing more.

Here's how you should use these to add a Product attribute:

// some InstallData.php

// get the builder
$attributeConfiguration = $objectManager->get(\Magento\Catalog\Setup\ProductAttributeConfiguration::class);

// configure it
$attributeConfiguration             
    ->withStoreScope()
    ->withFrontendLabel('Name')
    ->userDefined()
    ->visible()
    ->filterableInSearch(false)
    ->applyingTo(['simple'])
    ->withBackendType('varchar')
    ->withGroup('General')
    ->withFrontendInput($objectManager->get(\Magento\Ui\Component\Form\Element\DataType\Text::class));

// create the attribute using the new \Magento\Eav\Setup\EavSetup::addAttributeViaConfig()
$objectManager->get(\Magento\Eav\Setup\EavSetup::class)
              ->addAttributeViaConfig('shiny_new_attribute', $attributeConfiguration);

You should instantiate the builder of the EAV type you're interested in:

• CategoryAttributeConfiguration for category
• ProductAttributeConfiguration for product
• CustomerAddressAttributeConfiguration for customer address, and
• CustomerAttributeConfiguration for customer

---

Dilemma 1: You might not like that you need a full-blown instance of an UI Element when declaring a frontend input type, just so the code can extract whether it's text or multiselect etc.:

->withFrontendInput($objectManager->get(\Magento\Ui\Component\Form\Element\DataType\Text::class));

My argument is explicitness. You're assured your input type is there. As all explicit things go, it's more to type. If everybody hates it, I can add another method which accepts a string, but this is my alternative to having just a string param or having many similar methods (@Vinai's suggestion).

Dilemma 2: I'm not sure whether I've covered up all the cases with the attribute configs. I've been mostly looking at \Magento\Eav\Model\Entity\Setup\PropertyMapperInterface and \Magento\Eav\Setup\EavSetup::addAttribute() for keys to pick up.

---

Needless to say - but I'll say it anyway -, suggestions are welcomed.

[nevvermind]

I see that Code Sniffer from CI failed with 30 | ERROR | Missing function doc comment (amongst others). That means adding a docblock in a no-param constructor, and I have to put something. But that's just... terrible.

It's the reason you get code like this, or this, or wow, even this.

[nevvermind]

Is this PR correct - #4239?
Because if it is, I will have to add it here, too.

[vkublytskyi]

@nevvermind, thank you for contribution. Having OO way to declare attributes in setup scripts is a good improvement. However there are some details in implementation that should be avoided in Magento 2.

1. Inheritance should not be used for new code if possible. Objects composition is preferred way of code reuse.
2. State-full objects should be avoided as it may leads to unpredictable application state. This is especially important with shared objects retrieved with ObjectManager::get method. It's more safe to use approach as in PSR-7 and return new instance of configuration object for each method which changes attribute configuration.
3. In current releases new public methods are not recommended so it's better to not introduce Magento\Eav\Setup\EavSetup::addAttributeViaConfig now

[nevvermind]

Thanks for the feedback, @vkublytskyi. I'm ok with 1. and 3., but I'm not sure about 2.

How would you create those builders with no state? Isn't that what factory generation is for?

So you're saying to

public function visible($flag = true)
{
    $newConfig = clone $this;
    $newConfig->attributeConfig->setData('visible', (bool) $flag);
    return $newConfig;
}

I agree that, because the Obj Manager gets you a singleton by default, it's better to avoid state-fulness, but I find this to be a slight-overkill.

But I like it.

So how about a main Attribute Configuration object with withAdditionalConfiguration(AdditionalAttributeConfiguration $additionalConfiguration), in which:

interface AdditionalAttributeConfiguration
{
    public function toArray();
}

CatalogAdditionalConfiguration implements AdditionalAttributeConfiguration
{
    // all the methods here
}

CustomerAdditionalConfiguration implements AdditionalAttributeConfiguration
{
    // all the methods here
}

[Vinai]

Great stuff!
Not that it matters that much, but I prefer expressive methods like withFrontendInputText() over having to remember a class name like \Magento\Ui\Component\Form\Element\DataType\Text. For the former the IDE helps me by providing autocompletion.
In fact, type code strings like text are easier to read, and also would avoid coupling a specific implementation like using a hardcoded class name.
The validation that the class exists and can be found could be the job of the withFrontendInput() method, not the code calling it.

[nevvermind]

Yeah, I thought you wouldn't like it. But:

In fact, type code strings like text are easier to read, and also would avoid coupling a specific implementation like using a hardcoded class name.

This is not a case of coupling, but injection. Be it a string text or an object like \Magento\Ui\Component\Form\Element\DataType\Text, you still need to pass it, otherwise you're not declaring an input type for your attribute. I agree that they're easier to read, though. Btw, they're fixed, so it's similar to a class: link.

I think I'll fallback to withFrontendInput($type). It's just not obvious how can one find a valid frontend input. For example, I used withFrontendInput('label'), thinking it works, but got an exception (I recall \Magento\Eav\Model\Adminhtml\System\Config\Source\Inputtype\Validator doing the checking). That's one reason I preferred using a type hint: being explicit what input types are available, just by checking which classes implement \Magento\Framework\View\Element\UiComponentInterface. But maybe this is a documentation problem.

[vkublytskyi]

@nevvermind, I agree that solution with cloning of configuration may looks over-complicated but I believe that more stable code worth it. Another one point that we should be careful about with such approach is memory usage.

While withFrontendInputText() is more readable and provide handy code completion in IDE there is more important to provide possibility introduce new input types for any module. So withFrontendInput($type) looks more suitable.

Probably most flexible way to implement extensible input types list will be add new dependency like InputTypeProviderInterface. We may have several implementation which will work with xml configuration, or with full qualified class names, or with simple strings, or composite which combines several implementations in a chain.

[nevvermind]

Wow! We've gone from a simple array to a full module! 😜

Do you mean InputTypeProviderInterfaceFactory
And then some

• CompositeInputTypeProvider
• SimpleInputTypeProvider
• UiInputTypeProvider

a la \Magento\Checkout\Model\ConfigProviderInterface?

With calls like:

• $this->inputTypeProviderFactory->create()->get(\MyClass\Input::class)
• $this->inputTypeProviderFactory->create()->get('text')
• $this->inputTypeProviderFactory->create()->get('label_with_bold')

Does my code have to do that, or yours, when using the Attribute?
I'm just going to add withFrontendInput($type) and nothing more, right?

[vkublytskyi]

I don't see a reason to have special factory. So we may have InputTypeProviderInterface which will be injected in AttributeConfiguration constructor.

interface InputTypeProviderInterface
{
   /**
    * Check if provided type can be recognized as paricluar input type
    *
    * @param string $type
    * @return bool 
    */
   public function exists($type);

   /**
    * Convert provided string to value expected buy frontend_input consumers
    *
    * @param string $type
    * @return bool 
    */
   public function normalize($type);
}

class ChainedInputTypeProvider implements InputTypeProviderInterface
{
    /**
     * @var InputTypeProviderInterface[]
     */
    private $providers;
    ...
}
class SimpleInputTypeProvider implements InputTypeProviderInterface
{
   ...
}
class UiInputTypeProvider implements InputTypeProviderInterface
{
   ...
}

And than we will have code in AttributeConfiguration like

/**
* @param string $type
* @throws InvalidConfigurationException if specified type not supported
* @return $this
*/
public function withFrontendInput($type)
{
       if (!$this->inputTypeProvider->exists($type)) {
             throw new InvalidConfigurationException(sprintf('Input type "%s" is not supported', $type));
       }
       $this->attributeConfig->setData('input', $this->inputTypeProvider->normalize($type));
       return $this;
 }

Which can be used like $attributeConfiguration->withFrontendInput(\Magento\Ui\Component\Form\Element\DataType\Text::class) or $attributeConfiguration->withFrontendInput('text');

[nevvermind]

a) I didn't mean it to be a special factory, but an auto-generated one, as I don't want to inject an instance of something which may not be used (maybe some won't call withFrontendInput()). I guess I should use InputTypeProviderInterface\Proxy for this.

b) Must I create SimpleInputTypeProvider in this PR, or that's for a later time? I'm not sure how to normalize the input type when receiving a FQCN. I can't just instantiate them and call some method - that'll need some other interface/class. Or do you plan on having frontend_input_type = \Some\Class in DB? I'd rather just implement a UiInputTypeProvider and a composite one, wire them in di.xml and set a preference for the composite. Thus you can add later implementations with no disruption.

c) For the exists() implementation of UiInputTypeProvider, I'm planning on using Magento\Eav\Model\Adminhtml\System\Config\Source\Inputtype\Validator. Just a FYI.

Thanks very much for those snippets. They helped a lot. You actually wrote half of this PR. 😄

[vkublytskyi]

a) I didn't mean it to be a special factory, but an auto-generated one, as I don't want to inject an instance of something which may not be used (maybe some won't call withFrontendInput()). I guess I should use InputTypeProviderInterface\Proxy for this.

Good point. And yes, proxy could be used for this. But usage of proxy should be declared in DI config. Class should not expects proxy as a parameter type.

b) Must I create SimpleInputTypeProvider in this PR, or that's for a later time?

No. We just need have an extension point and remove knowledge about ui component from attribute configuration. Single implementation would be enough.

[nevvermind]

Pushed new version. How about now?

$productAdditional = $objectManager->get(\Magento\Catalog\Setup\AttributeConfiguration\CatalogConfiguration::class)
                                   ->searchable()
                                   ->usedForSortBy();

$attrConfiguration = $objectManager->get(\Magento\Eav\Setup\AttributeConfiguration\MainConfiguration::class)
                                   ->withAdditionalConfiguration($productAdditional)
                                   ->required()
                                   ->withFrontendLabel('Label')
                                   ->withFrontendInput('text');

$objectManager->get(\Magento\Eav\Setup\EavSetup::class)
              ->addAttribute(
                  \Magento\Catalog\Model\Product::ENTITY,
                  'shiny_new_attribute',
                  $attrConfiguration->toArray()
              );

[nevvermind]

Maybe we can use this to hide the differences between addAttribute and updateAttribute, too (see #1917). But it would really help if we'd have something like addAttributeViaConfig and updateAttributeViaConfig, for type hinting, so that we can set up the builder in these methods, otherwise clients need to do that themselves - and it's already quite explicit as it is.

Besides, having those two new methods above will make future refactoring easier. I understand why you wouldn't want APIs to change, but concrete implementations, too? Why?

[piotrekkaminski]

hey @nevvermind the Travis seems to be failing on something weird. Can you update to latest dev branch and resubmit?

[piotrekkaminski]

Internal issue MAGETWO-53266

[adragus-inviqa]

@piotrekkaminski - it already was rebased on the latest. The reason CI's failing is not weird, but it has exactly the same type of error as the previous. Somebody's already starting to code against 5.6.

[piotrekkaminski]

Sorry @adragus-inviqa you are right, we had different failures before. We will update Travis to use PHP 5.6

[nevvermind]

Inheritance should not be used for new code if possible. Objects composition is preferred way of code reuse.

@vkublytskyi - the more I look back at this code, the more it seems wrong to use composition.

a) A customer attribute configuration (CustomerConfiguration) is an attribute configuration (MainConfiguration). A catalog attribute configuration (CatalogConfiguration) is an attribute configuration (MainConfiguration). They're specialised classes.

b) Classes are additive, they're not some other kind of behaviour.

c) I don't think M2 will work if you don't provide an additional configuration, so having a setter is counter-intuitive. A constructor arg would be better - in the case of composition, anyway - but I still think composition is wrong in this case.

Is there a reason why we shouldn't switch back to inheritance?

[vkublytskyi]

@nevvermind, Agree that there should not be possible to create object that is inconsistent at some point. Also for me MainConfiguration should not know anything about any additions. But some specialized configurations may require MainConfiguration as constructor argument.

For now inheritance looks more native. But it may restrict us in future. E.g. we may have some overlapped sets of attributes that are suitable for different entities. Even now we have duplicated visible method. With composition we may have VisibilityConfiguration which accepts any implementation of ConfigurationInterface so it may be applied to CustomerConfiguration or to CatalogConfiguration.

[nevvermind]

I didn't quite get the VisibilityConfiguration example, but ok, I'll amend according to your suggestions. Thanks again.

[nevvermind]

Refactored. Additional configuration objects now need a MainConfiguration instance.

[vkublytskyi]

No reason to clone main configuration here as it's immutable so safely may be shared by several instances

[nevvermind]

Thanks for reviewing. Amended.

[nevvermind]

Can I bother someone to review this again?

[vkublytskyi]

@nevvermind, one more thank you for your PR request and your hard work. And sorry for the delay in response.
After long discussions, we've dicided to not merge this changes to mainline. Main reason is that this code creates a configuration for Magento\Eav\Setup\EavSetup which is not declared as module API. Magento strives to build solid API which may be used by modules to interact with each other or external systems for integration (REST/SOAP). So we came up with the solution that Magento\Eav\Api should provide all functionality necessary for EAV configuration.

Now we are working on the design for EAV API and will update you here when it ready. Please share any thoughts on this.

One more time thank you for your impact. It raises important issue that should be solved in Magento.

[nevvermind]

No worries. At least I got you to update things, maybe to a more declarative approach, far from array keys. :)

I don't mind this PR being closed, btw. I'll hopefully see the changes in the CHANGELOG when they arrive; no need to update this thread.

[ghost]

Opened internal task item MAGETWO-56718

[okorshenko]

Thank you @nevvermind for your contribution and patience. This Pull Request is a long and very interesting story :)