Skip to content

Add Value to Magento Framework's public API #17296

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Oct 25, 2018
Merged

Add Value to Magento Framework's public API #17296

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9f47f05
Add Value and ValueInterface to Magento Framework's public API
navarr Jul 31, 2018
e24d8b9
Fix Value model description
slavvka Oct 17, 2018
863f772
Remove API annotiotion
slavvka Oct 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions lib/internal/Magento/Framework/App/Config/Value.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@
/**
* Config data model
*
* This model is temporarily marked as API since {@see \Magento\Framework\App\Config\ValueInterface} doesn't fit
* developers' needs of extensibility. In 2.4 we are going to introduce a new iterface which should cover all needs
* and deprecate the mentioned together with the model
*
* @method string getScope()
* @method \Magento\Framework\App\Config\ValueInterface setScope(string $value)
* @method int getScopeId()
Expand All @@ -17,6 +21,8 @@
* @method string getValue()
* @method \Magento\Framework\App\Config\ValueInterface setValue(string $value)
*
* @api
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's a good idea to make fat ugly model a part of API. Why isn't interface sufficient?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When writing a backend model for a config, Magento expects that the model implements methods that do not exist in the Interface (such as "addData"). Though, this can be solved by any implementation of ValueInterface extending from AbstractModel (as Value does).

However, this model contains all the default functionality expected from a configuration backend model - including setting the correct events to fire and invalidating the cache. I have hard trouble finding a backend model that does not extend from Value.

Tutorials and examples online also show that the typical config backend model utilizes this model. I would think for those reasons it should be part of the public @api so that custom module's backend models may depend on it not removing functionality without a major version break.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you do not use the Value model, you must also specify the resource and resourcecollection for configuration yourself. Magento does it for Value as such:

    <type name="Magento\Framework\App\Config\Value">
        <arguments>
            <argument name="resource" xsi:type="object">Magento\Config\Model\ResourceModel\Config\Data</argument>
            <argument name="resourceCollection" xsi:type="object">Magento\Config\Model\ResourceModel\Config\Data\Collection\Proxy</argument>
        </arguments>
    </type>

It appears that in all cases the expectation is that you extend from this model

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@navarr do we have any other model extending AbstractModel marked as @api?

Copy link
Member Author

@navarr navarr Aug 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@orlangur Yes. For example \Magento\Customer\Model\Customer and Magento\Eav\Model\Entity\Type. Nothing in Magento\Framework, however.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently we have the interface for this model marked with @api. There's no sense to mark the model itself as @api as well since we won't be needed the interface. That's why it is important to extend the interface while we can deliver that to 2.3.0

Copy link
Member

@slavvka slavvka Oct 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@navarr If you you're still interested in delivering this and your other PR #17300 to 2.3.0 please answer me

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@slavvka I've had little personal time to work on open source - so forgive me for the delay - though I had last spoken about this over two months ago.

I laid out a variety of good reasons that this model should be part of the API on August 1st, and that simply updating the interface is not good enough. My fallback position on this is that "developers currently rely on this model to perform the basic tasks associated with a backend value"

And similarly Magento expects that it is receiving a Value model for a vast majority of the information it conveys to such objects:

// Magento\Config\Model\PreparedValueFactory
            /** @var ValueInterface $backendModel */
            $backendModel = $this->valueFactory->create(
                $backendModelName,
                ['config' => $this->config]
            );

            if ($backendModel instanceof Value) {
                $scopeId = 0;

                $scope = $this->scopeTypeNormalizer->normalize($scope);

                if ($scope !== ScopeInterface::SCOPE_DEFAULT) {
                    $scopeId = $this->scopeResolverPool->get($scope)
                        ->getScope($scopeCode)
                        ->getId();
                }

                if ($field instanceof Structure\Element\Field) {
                    $groupPath = $field->getGroupPath();
                    $group = $structure->getElement($groupPath);
                    $backendModel->setField($field->getId());
                    $backendModel->setGroupId($group->getId());
                    $backendModel->setFieldConfig($field->getData());
                }

                $backendModel->setPath($configPath);
                $backendModel->setScope($scope);
                $backendModel->setScopeId($scopeId);
                $backendModel->setScopeCode($scopeCode);
                $backendModel->setValue($value);
            }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since current interface is not enough and we are in code-freeze stage now, I propose the following:
In 2.3.0

  1. Mark the model as @api
  2. Do NOT mark the interface as @api

In 2.4.0

  1. Create a task to investigate how the interface should look like to cover the needs
  2. Extend existing interface or create a new one based on the investigation. Mark it as @api
  3. Deprecate the model

in 2.5.0

  1. Remove @api from the model and leave only one interface marked as @api

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, the decision is made. We are following to what @paliarush described. Please remove @api mark from the interface and add there a description what is going to be done with it. Also please add a description to the model that what it will be marked as @api temporarily until we introduce a new interface and make everybody use use it.

*
* @SuppressWarnings(PHPMD.NumberOfChildren)
*/
class Value extends \Magento\Framework\Model\AbstractModel implements \Magento\Framework\App\Config\ValueInterface
Expand Down
5 changes: 4 additions & 1 deletion lib/internal/Magento/Framework/App/Config/ValueInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,10 @@

/**
* Interface \Magento\Framework\App\Config\ValueInterface
*
*
* This interface cannot be marked as API since doesn't fit developers' needs of extensibility. In 2.4 we are going
* to introduce a new iterface which should cover all needs and deprecate the this one with the model
* {@see \Magento\Framework\App\Config\Value}
*/
interface ValueInterface
{
Expand Down