Skip to content

Add @api annotation to the ScopeConfig WriterInterface #14374

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

Merged
merged 1 commit into from
Mar 29, 2018
Merged

Add @api annotation to the ScopeConfig WriterInterface #14374

merged 1 commit into from
Mar 29, 2018

Conversation

navarr
Copy link
Member

@navarr navarr commented Mar 26, 2018

Description

I would like a more official way of writing config that's part of the API.

Currently, You should use Magento\Config\Model\ResourceModel\Config::saveConfig - which is @api annotated.

That class implements Magento\Framework\App\Config\ConfigResource\ConfigInterface (which is not annotated) - and that class is used by Magento\Framework\App\Config\Storage\WriterInterface (which is not annotated - but I propose to do so here)

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)

@magento-engcom-team
Copy link
Contributor

Hi @larsroettig, thank you for the review.
ENGCOM-1097 has been created to process this Pull Request

@magento-engcom-team
Copy link
Contributor

@navarr thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

@navarr
Copy link
Member Author

navarr commented Mar 26, 2018

@magento-engcom-team That link just takes me to the magento user page on github.

@larsroettig
Copy link
Member

@navarr Because you are already in Magento org :-) maybe we should fix this in automation 🔍

@sidolov
Copy link
Contributor

sidolov commented Mar 27, 2018

Hi @navarr , I see that in namespace \Magento\Framework\App\Config we have several interfaces without @api annotation, so, can you describe the scenario when you need write to storage with usage \Magento\Framework\App\Config\Storage\WriterInterface?

@navarr
Copy link
Member Author

navarr commented Mar 27, 2018

@sidolov Yes. Our team was writing a log rotator (for the database), where older logs are deleted.

To accomplish this, we had two config fields: a start time, and a frequency. These are then compiled into a configuration setting that contains the actual cron format.

We needed a method to write to the config during this compilation step. The developer's first go was to use the config Value model and save it - but that's not the Magento way, so we went down the rabbit hole of trying to figure out how we're supposed to save config.

The internet likes WriterInterface - but it's not annotated, so I went down the chain of things it does to find something annotated.

Unfortunately what we found was Magento\Config\Model\ResourceModel\Config. While we can use that (since it's annotated) - using another module's resourcemodel seems very wrong.

The ideal situation is that there is an api annotated interface for setting config. WriterInterface seems like the best choice.

@sidolov
Copy link
Contributor

sidolov commented Mar 28, 2018

@navarr I see that it useful for you, thank you for fast response!

@magento-engcom-team magento-engcom-team merged commit c6c02e2 into magento:2.3-develop Mar 29, 2018
@navarr navarr deleted the patch-13 branch April 10, 2018 19:34
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.

4 participants