jolicode / elastically

🔍 JoliCode's Elastica wrapper to bootstrap Elasticsearch PHP integrations
248 stars 37 forks source link

Consider use of DependencyInjection\Loader\Yaml instead of Yaml::parseFile #54

Closed insekticid closed 3 years ago

insekticid commented 3 years ago

Use Symfony\Component\DependencyInjection\Loader\Yaml::load instead of Symfony\Component\Yaml\Yaml::parseFile and you can use benefits like imports. It can be useful when extending from another base mapping file

https://github.com/jolicode/elastically/blob/64cba836d9c8248d4afa9e02f6a3283f68aee6a6/src/IndexBuilder.php#L32 simply check if class_exists('Symfony\Component\DependencyInjection\Loader\Yaml') and use it if available

content_base_mapping.yaml

mappings:
  dynamic: false
  properties:
    name:
      type: text

cs_content_mapping.yaml

imports:
  - { resource: 'content_base_mapping.yaml' }

de_content_mapping.yaml

imports:
  - { resource: 'content_base_mapping.yaml' }
damienalexandre commented 3 years ago

Hi!

Thanks for the suggestion, I agree this can bring a lot of convenience!

We could just patch the IndexBuilder right away but I think \Symfony\Component\DependencyInjection\Loader\YamlFileLoader::load was not created for this purpose. It's supposed to parse "services" and "parameters" too (+ extensions). It does more that just parsing YAML & importing. Have you tried it?

Your proposal is also a good reminder that we need to change the way mapping definition are loaded. I track this need in #22 - the objective it so allow any source for the mapping definition, not just pure YAML file.

lyrixx commented 3 years ago

Hello.

As @damienalexandre said, I don't think it's possible since the class https://github.com/symfony/symfony/blob/5.4/src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php is tied to a container and many other stuff (It valides the YAML, it load services, etc...)

I opened #79 to allow more mapping provider. Feel free to contribute if you find something better. We'll super happy to merge it 👍🏼

In the meantime, I'm going to close this issue since it's not actionable.

Thanks for the suggestion :)