Skip to content
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.

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

Proposal for configuration provisioning #370

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

sydekumf
Copy link

Problem

When you start a new Magento project there are similar initial tasks to set up configuration including websites, stores, tax, store configuration, attributes and more which lead to different problems:

  • Boilerplate Code in data patches (copied from Google, DevDocs, StackOverflow, other projects, maybe poorly adjusted)
  • Update and Delete operations are tedious and complex as you have to manage relations and identification inside data patches (i.e. store code vs. autoincrement id)
  • Installation from scratch with scope level configuration inside config.php is not possible if you rely on it in data patches

Solution

There should be a declarative schema approach to provision a new instance with all necessary configuration to run. It is related to the existing delcarative database schema but manages configuration data like:

  • Websites
  • Stores
  • Store Groups
  • System configuration
  • Attribtues
  • Attribute Sets
  • Attribute Groups
  • Tax

Requested Reviewers

@buskamuza
@paliarush

@buskamuza
Copy link
Contributor

I agree that the declarative approach is always better than imperative, and having declarative data scripts has drawbacks. So having declarative data definitions would be a huge improvement. Regarding the current proposal:

  1. I would change terminology from "configuration" to "data". What is described is storing entities in the storage (web sites, stores, etc). So those are the same things as the ones handled by data scripts currently.
  2. The challenging part, as also mentioned in the document, is the modifications. Here we would need to cover questions like "what happens if the data has already been removed when we try to update/delete it?", "how to declare modifications that rely on existing data" (something that can't be declarative). In general, I would first question the necessity of the declarative data for upgrade scenario. From the problem statement, it looks like we need these declarations for initial setup only. In this case install scenario will be sufficient. It would reduce complexity of the solution, IMO. Then data scripts will continue performing the role of "modifiers" (they would modify existing data), while declarative data would be executed only once. It can be during installation, or it might be ok to apply data during setup:upgrade, just to make sure it's only once.
  3. It would be good to cover whether existing data scripts should be transformed in the declarative representation. Or whether only some of the upgrades should be converted.
  4. If target audience for this feature is SIs, maybe it the framework should support location of those declarations in app/etc (as part of a specific project). While they can also be located in modules to support essential data (like websites), w/o which the module can't work.

@buskamuza
Copy link
Contributor

buskamuza commented May 22, 2020

Scheduled meeting to discuss: https://adobe.bluejeans.com/7385677850 , Thursday, May 28, 10am (CDT)

@vgoncharenko
Copy link

@mslabko I guess profile generator has too general approach for generation. entities like 'Store 1' and 'Store 2' rarely used in real online stores. @sydekumf by the way, what is the difference between data import and this approach? The names of entities would be different each time anyway. CSV is not so hard to manage comparison to XML/JSON or whatever format you would like to pick. And even if import isn't the option, you always can create own sample-data configuration and use it. if I'm not mistaken it use CSV config types as well.

@akaplya
Copy link
Contributor

akaplya commented May 28, 2020

It could be quite uncertain. Due Magento saves the mentioned entities in a database the introduction of config like this will cause the question is this data that were declared can be edited?
What should happen if the module disabled or removed?

The common recommendation never setup new EAV attributes through the application installation because the stores, attributes, and sets are something that should be managed by a merchant and we can not rely on this information presence in code.

The proper solution for the case you mentioned will be data import.

@buskamuza
Copy link
Contributor

Meeting minutes:

  • Vadim Justus: provision entity data, it is not the one time scenario. Sometimes new requirements come from 3rd-party systems, so data should be updated/added.
  • Stepan: Config dump is a mixed approach, has bugs. Config is applied after data patches, so if a patch relies on data in the config, it fails.
  • Kril: Stores, attributes (and other data mentioned) are considered code and are project-specific.
  • Florian: data (config) definitions should be mergeable in modules.
    • Kaplia: this will create conflicts with data provided manually via Admin Panel.
    • See maintained flag.
  • Kaplia: use import?
    • Some attributes are necessary for business logic.
  • Kril: only "system" attributes can be declared in code.
  • Kril: extensions should not create websites/stores/some other data. These are project specific.
  • Olga: it would be good to specify which data should be managed from module, and which should be on project level. The proposal should be specific to entities, not generic.
  • Kaplia: what about attribute sets?
    • Kril: put in Default.
  • Kril: there needs to be an abstraction for entity attributes.
  • Merchants usually done't create attributes or attribute sets from Admin Panel. But there are a lot of attributes and rules are change because of new PIM rules.
  • Kaplia: cover behavior in case of addition of required attribute after the system already exists.
  • Should we prohibit attributes modification from Admin Panel if there are declarations for attributes?
    • Stepan: no, because as soon as an extension with declarative data is installed, UI becomes unavailable.
  • Kaplia: what happens with the new system attribute which is added to existing system, when there are already attribute sets?

Action items:

  • Describe module vs project declarations.
  • Distinguish specific entities and how the behavior should look like for each of those. Start from a few important entities.
  • For attributes, take into account addition of required attributes (should it be prohibited? should it require default value? other?)
  • Describe in more details what happens if admin edits/adds/deletes same data from Admin Panel.

@akaplya
Copy link
Contributor

akaplya commented May 28, 2020

@sydekumf one more question, in the examples you mentioned could EAV attributes be replaced with extension attributes which are already declarative?
https://devdocs.magento.com/guides/v2.3/extension-dev-guide/attributes.html#extension

@sydekumf
Copy link
Author

sydekumf commented Jul 3, 2020

Hi @buskamuza we updated the proposal with the information from our last call :-) Could you please have a look into it. We would be very happy to talk about that with you in another call. Let me know what you think, thanks a lot!

On top of that every configuration type module defines a `maintained` flag which basically defines if the configuration can be overriden by a user or not. If it is set to `yes` the configuration provisioning will always override the values, independently if it was changed or not.
There is also a field `is_mutable` which defines if the declared data can be changed by the admin. The following explains the behavior:

* `is_mutable` is not defined:
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to have the checkbox described more clearly, for somebody w/o context it's not very clear which checkbox is mentioned. As I understand, it's a feature similar to current "system" value? Maybe just move this "is_mutable" flag is checkbox at the attribute grid and at attribute edit page higher in the document, before "checkbox" is mentioned?

On top of that every configuration type module defines a `maintained` flag which basically defines if the configuration can be overriden by a user or not. If it is set to `yes` the configuration provisioning will always override the values, independently if it was changed or not.
There is also a field `is_mutable` which defines if the declared data can be changed by the admin. The following explains the behavior:

* `is_mutable` is not defined:
Copy link
Contributor

Choose a reason for hiding this comment

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

There are 3 options for is_mutable values: true, false, locked. Which one corresponds to "not defined"?

- "**is_mutable**" flag is checkbox at the attribute grid and at attribute edit page
- Attributes which created from Admin panel is not maintainable by xml (no dumping, "**is_mutable**" flag is not shown but has true value in database) until xml with the same attribute code will be created and "**is_mutable**" will be unchecked
- "**is_mutable**" flag is responsible for defining maintenance responsibilities:
- **true** responsibility on Admin panel side switchable from admin
Copy link
Contributor

Choose a reason for hiding this comment

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

Why true and false options are needed? Why would we need for admin to check/uncheck the checkbox?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd reiterate on description for "is_mutable". It looks a little bit confusing right now, from different sentences.
Would it even make sense to divide it into two options?

  1. is_mutable (true, false) - if true, admin can modify the value (which will override what's declared in the file), if false, admin can't modify it
  2. revert to system value - a button/checkbox for mutable attributes in Admin UI to revert to system value (declared in the file)

Case of removal should be also covered. Should it be possible for admin to remove a mutable entity/attribute? In this case, should there be UI for restoring it from the system value?


## Created by admin (xml declaration doesn't exists):
- Fully managed by admin panel
- If xml with the same attribute code will be created - it will take over all responsibilities to a code side and will be then managed like attribute created by XML declaration
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this should be tru only for non-mutable entities? Though you may provide more real-world scenarios to understand what would be more convenient.
For me described behavior makes things more complex. Like, if I'm looking at an instance (installed by somebody some time ago), how do I know if it's a feature or a bug that some values are taken from files, and others are not (probably Admin entered them). I would prefer to not rely on time for the application logic as it's difficult to reason about.

@sydekumf
Copy link
Author

We simplified the concept and updated the document :-)


If XML with the same attribute code will be created (because it was created by admin first) it will take over all responsibilities to a code side and will be then managed like attribute created by XML declaration.

**Optional behaviour:** If admin unchecked the checkbox and changes the entity, the data can be restored if the admin checks the checkbox again. The data from XML delcaration will then restore the entity. This behaviour is optional as it is not really necessary regarding a provisioning functionality, but of course nice to have.
Copy link
Contributor

Choose a reason for hiding this comment

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

When the data is restored? At the moment the admin unchecks the checkbox or during next update? If at the moment of the admin action, won't be it too destructive to the store?

@buskamuza
Copy link
Contributor

@fascinosum : How events on configuration update will be covered? May be not an issue for initial installation, but may lead to unexpected behavior during update.

@buskamuza
Copy link
Contributor

Regarding required attributes: agreed that as this is an existing problem, it's not necessarily should be addressed in this proposal. A default value should be used for attributes whenever possible, in this case it can be used to fill data for existing records.


As already mentioned, the data structure is related to the existing database declarative schema. Every module can define a XML file for a particular configuration type. All configuration files for the same type would be merged to a single logical structure. Therefore every configuration type module should at least support merge logic. This is done individually, as for example, websites are merged differently than attributes.

Therefore, a virtual type of `Magento\Framework\Config\Reader\Filesystem` will be defined, which has a ` Magento\Framework\Config\ConverterInterface` which is responsible for merging the XML files.
Copy link
Contributor

Choose a reason for hiding this comment

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

Magento\Framework\Config\ConverterInterface has a different purpose. XML merge implemented in \Magento\Framework\Config\Dom::merge. Merge is a straightforward process without complex logic

Copy link
Author

Choose a reason for hiding this comment

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

"Merging" is not a plain XML merge. As one sentence above said, every module for itself is responsible HOW to merge as websites will be merged differently than attributes. That's why we just can't simply merge XML with \Magento\Framework\Config\Dom::merge.

See this for an example: https://github.com/magento-hackathon/m2-content-provisioning/blob/develop/etc/di.xml#L65

@sydekumf
Copy link
Author

@buskamuza How can we proceed with this PR? Would be happy to get any news or plans from Adobe/Magento :-) Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants