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

[RFC] New configuration parsing model to manage everything with Symfony services #1072

Open
Aeliot-Tm opened this issue Dec 7, 2022 · 7 comments

Comments

@Aeliot-Tm
Copy link

Aeliot-Tm commented Dec 7, 2022

Q A
Bug report? no
Feature request? yes
BC Break report? no
RFC? yes
Version/Branch x.y.z

I creates this issue to have single place to collect information about the new model of the parsing and caching of GraphQL types configs. As I know, the work was started by @Vincz and this was discussed and validated by @mcg-web at the time. But I cannot find comprehensive information about it to help with the finishing of the work. The code base only.

So, below is the information collected so far.
It would be nice when somebody provide me more information.

  1. The common API
    At the moment, the configuration parsers parse configuration and generate an array of config. This array of configuration is then validated with a Yaml configuration.
    The idea of the new way to do it, is to describe an API. This API describe a schema (all the classes under https://github.com/Vincz/GraphQLBundle/tree/master/src/Configuration).
    The various parsers are considered as ConfigurationProvider. We don't know how they work, we just want them to generate a Configuration.
    For example, the Metadata parser (https://github.com/Vincz/GraphQLBundle/tree/master/configuration/GraphQLConfigurationMetadataBundle) will use annotations and attributes to generate this Configuration object while the YAML parser (https://github.com/Vincz/GraphQLBundle/tree/master/configuration/GraphQLConfigurationYamlBundle) will read a YAML file to generate this configuration.
    So if we want to create a new type of Parser or whatever service that can generate a Configuration object we can.
    We do not duplicate anything. The parsers currently in https://github.com/Vincz/GraphQLBundle/tree/master/configuration will be distinct packages, and we install only the ones we want.
    An other advantage of doing it this way, is the ability to use various type simultaneously (like YAML + Annotations for example) and use type of others parsers. It is not possible with the current version.

  2. Use a service
    With the current version, the parsing is part of the cache warming process. It prevents us from using all Symfony services related stuff. The idea of the new configuration is to manage everything with Symfony services and to generate the Configuration object when it's needed (and cache it). You can check here https://github.com/Vincz/GraphQLBundle/blob/master/src/Resources/config/configuration.yaml for the declaration of the main Configuration object. It is generated by the ConfigurationProvider using all configured providers.

You can check the latest version by @Vincz here : https://github.com/vincz/GraphQLBundle
And my work it here: https://github.com/Aeliot-Tm/GraphQLBundle/tree/dynamic_config_parsing

@Aeliot-Tm Aeliot-Tm changed the title [RFC] New configuration to manage everything with Symfony services [RFC] New configuration parsing model to manage everything with Symfony services Dec 7, 2022
@Aeliot-Tm
Copy link
Author

Aeliot-Tm commented Dec 8, 2022

Progress updates:

  • fixed passing of tests in the version by @Vincz
  • merged last master branch

I work on tests passing.

@Aeliot-Tm
Copy link
Author

Aeliot-Tm commented Dec 8, 2022

Progress updates:
PHPUnit tests passed!!!
But I temporary rolled back getting of service overblog_graphql.cache_compiler from the container in the \Overblog\GraphQLBundle\DependencyInjection\Compiler\TypeGeneratorPass. (described in the issues: #899)

So, plans:

  • First of all, I'm going to fix defining of services definitions or to do some workaround.
  • Then I'm going to check what was not finished and what came from master branch and should be refactored.

@Aeliot-Tm
Copy link
Author

Aeliot-Tm commented Dec 8, 2022

And I would like to consider namespaces changing of new configurations packages.
I think, they should be changed:

  • There should be removed duplicated work "Bundle".
  • And reduced count of words.

For example:
image

@mcg-web How do you think?

@Aeliot-Tm
Copy link
Author

@mcg-web Why are generated types defined as services? I see that it is the initial idea, but what are the main pros of it now? As I can see, this is only for injecting them together with all other types defined in any bundle.

If it so, I'd like to refactor this part.
My the main idea is to build generated types "on demand" based on the information from the types config \Overblog\GraphQLBundle\Configuration\Configuration. And leave opportunity of other types collecting as is.

It permits to avoid getting of the service overblog_graphql.cache_compiler from container (see #1072 (comment)) in compiler pass. And we don't need to get Overblog\GraphQLBundle\ConfigurationProvider\ConfigurationProvider in a similar way. The main reason of it is fact that we don't control configuration providers (tag: overblog_graphql.configuration.provider) with the new model.

@Aeliot-Tm
Copy link
Author

@Vincz
Copy link
Collaborator

Vincz commented Dec 12, 2022

Hi @Aeliot-Tm!
First of all, I'm really sorry for my late reply. I've got a lot in my plate this days with a bunch of projects ending and it's kinda of the rush everyday... So, sorry for that.
It's really awesome that you're continuing the configuration refactoring.
Regarding the change of namespace, no problem for me if @mcg-web agree.
And yes, the statement in DefautValueTrait is debug that you can get rid of without problem.
If you have more questions about my implementation, feel free to ask me and I'll answer as soon as I can.
Now that I'm thinking about it, I was also working on the extensions refactoring (https://github.com/Vincz/GraphQLBundle/tree/master/src/Extension) using the new Configuration mechanism and a more Symfony service with proper configuration here also. I think this part will also need to be handled as it is necessary to keep the builder for example (but I think it was working already and was BC with the old one).

@Vincz
Copy link
Collaborator

Vincz commented Apr 4, 2024

Hi @Aeliot-Tm! Are you still working on this? I'm planning to dedicate a bit of time working on it. Did you commit what you did somewhere?

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

No branches or pull requests

2 participants