Remove autowiring #285
Remove autowiring #285
Conversation
| @@ -31,7 +31,6 @@ protected function configureContainer(ContainerBuilder $container, LoaderInterfa | |||
| } else { | |||
| $loader->load(__DIR__.'/config/config_php7.yaml'); | |||
| } | |||
| $loader->load(__DIR__.'/../config/services.xml'); | |||
There was a problem hiding this comment.
This was wrong, because the MeilisearchBundle loads the services, so this was just overrode always what the bundle defined, that means what we set in MeilisearchExtension was lost and many tests were failing because the configuration was not properly injected to the SearchService.
| { | ||
| $this->normalizer = $normalizer; | ||
| $this->engine = $engine; | ||
| $this->configuration = new Collection($configuration); | ||
| $this->propertyAccessor = PropertyAccess::createPropertyAccessor(); | ||
| $this->propertyAccessor = $propertyAccessor ?? PropertyAccess::createPropertyAccessor(); |
There was a problem hiding this comment.
Symfony defines a property accessor already, so we can inject it through DI.
norkunas
force-pushed
the
remove-autowiring
branch
2 times, most recently
from
7b29a3d to
f8da79c
Compare
|
@brunoocasali not sure I think that it needs to use a different config without any subscribed events, to test each event separately? |
norkunas
force-pushed
the
remove-autowiring
branch
3 times, most recently
from
d9f4dc5 to
dd4483e
Compare
| <argument /><!-- url --> | ||
| <argument /><!-- api key --> | ||
| <argument type="service" id="psr18.http_client" on-invalid="ignore" /><!-- http client --> | ||
| <argument>null</argument><!-- request factory --> | ||
| <argument /><!-- client agents --> | ||
| <argument>null</argument><!-- stream factory --> |
There was a problem hiding this comment.
Had to rework this as something was not right on Symfony 5.4: Symfony\Component\DependencyInjection\Exception\RuntimeException: Invalid constructor argument for service "meilisearch.client": integer expected but found string "url". Check your service definition.
| @@ -17,10 +17,10 @@ public function getConfigTreeBuilder(): TreeBuilder | |||
|
|
|||
| $rootNode | |||
| ->children() | |||
| ->scalarNode('url')->end() | |||
| ->scalarNode('url')->cannotBeEmpty()->end() | |||
There was a problem hiding this comment.
This is like a BC break.. but Meilisearch\Client expects a string for the base url and if it was not set then MeilisearchExtension was setting argument to null which does not make sense.
There was a problem hiding this comment.
I agree. Not adding the URL means the library will not work. Unless somewhere else we have the localhost being passed automatically.
There was a problem hiding this comment.
Hm, we could set http://localhost:7700 as default value then, at least it would not break app in compilation time
There was a problem hiding this comment.
It should be written somewhere then :)
| <argument key="$clientAgents" type="collection"> | ||
| <argument>%meili_symfony_version%</argument> | ||
| </argument> | ||
| <argument /><!-- url --> |
There was a problem hiding this comment.
I'm curious, now we don't need to add the key=""?
There was a problem hiding this comment.
In the src/DependencyInjection/MeilisearchExtension.php, I saw that you are changing to use a positional instead of a named argument. How is this better?
There was a problem hiding this comment.
After PR, I left the comment about it :) #285 (comment)
norkunas
force-pushed
the
remove-autowiring
branch
from
dd4483e to
6f0fcc2
Compare
norkunas
force-pushed
the
remove-autowiring
branch
5 times, most recently
from
2a704fb to
ab4c3f4
Compare
norkunas
force-pushed
the
remove-autowiring
branch
from
ab4c3f4 to
b84b762
Compare
|
bors merge |
Pull Request
Related issue
Fixes #279
What does this PR do?
PR checklist
Please check if your PR fulfills the following requirements: