-
Notifications
You must be signed in to change notification settings - Fork 29
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
Upgrade to Symfony v5.4 minimum #265
Conversation
@norkunas should rector be kept (for later usage?) |
@brunoocasali should decide, but in case we need it very rarely if at all, maybe we shouldn't introduce as dev dependency to save some resources |
By the way Symfony 6.3 should be added to matrix |
Oki will do indeed the new 6.3 |
I'm not opposed to keeping it, but my go-to opinion about this is: not to add new dependencies, even dev ones. So if we can remove it let's do it! :) |
@brunoocasali @norkunas friendly ping, PR ready for reviews :) |
lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bors merge
Can you check the CI error @94noni? 😇 |
i will have a look at it indeed, hopefully soon :) |
Hi @94noni, it is because you never got a PR merged. As soon you merge your first one, the CI will automatically run :) it is just a way to avoid spam (I think so)! |
6.1.* fails, I suggest to leave overriding |
indeed, with the swap to attribute, but the bundle is not sf6+ so we will need to add those back yes |
8a66f9e
to
5f9dd09
Compare
can you try |
bors try |
We should merge #266 before |
Yeah, I was trying to force this CI action to trigger but had no success, either! Let's fix the conflicts then after approved we can see what is happen. |
as #266 will hopefully be merged shortly, I will rework on this right after |
266: Use meilisearch prefix for services r=brunoocasali a=94noni # Pull Request ## What does this PR do? - Improves "best practices/coding standard" as the « search » prefix is to generic, lets be specific for this bundle This PR can be merged before #265 (as it will require a major release) TODO: Assert that the container has all services and that deprecations are triggered - https://symfony.com/doc/5.4/bundles/best_practices.html#services - https://symfony.com/doc/5.4/service_container/alias_private.html#deprecating-service-aliases --------- # RFC I found some inconsistency reading the repo: - Commands namespace: `meili:` - Bundle config: `meili_search` - Services: `search` (this PR) Will it be nice to define them as `meilisearch`everywhere? Co-authored-by: Antoine Makdessi <[email protected]>
@brunoocasali seems a failure in the merge, will wait a little bit to restart this thx ! |
266: Use meilisearch prefix for services r=brunoocasali a=94noni # Pull Request ## What does this PR do? - Improves "best practices/coding standard" as the « search » prefix is to generic, lets be specific for this bundle This PR can be merged before #265 (as it will require a major release) TODO: Assert that the container has all services and that deprecations are triggered - https://symfony.com/doc/5.4/bundles/best_practices.html#services - https://symfony.com/doc/5.4/service_container/alias_private.html#deprecating-service-aliases --------- # RFC I found some inconsistency reading the repo: - Commands namespace: `meili:` - Bundle config: `meili_search` - Services: `search` (this PR) Will it be nice to define them as `meilisearch`everywhere? Co-authored-by: Antoine Makdessi <[email protected]>
266: Use meilisearch prefix for services r=norkunas a=94noni # Pull Request ## What does this PR do? - Improves "best practices/coding standard" as the « search » prefix is to generic, lets be specific for this bundle This PR can be merged before #265 (as it will require a major release) TODO: Assert that the container has all services and that deprecations are triggered - https://symfony.com/doc/5.4/bundles/best_practices.html#services - https://symfony.com/doc/5.4/service_container/alias_private.html#deprecating-service-aliases --------- # RFC I found some inconsistency reading the repo: - Commands namespace: `meili:` - Bundle config: `meili_search` - Services: `search` (this PR) Will it be nice to define them as `meilisearch`everywhere? Co-authored-by: Antoine Makdessi <[email protected]> Co-authored-by: Tomas Norkūnas <[email protected]>
bors merge |
Hello @94noni can you rebase your branch please? 😊 |
protected static $defaultName = 'meilisearch:clear|meili:clear'; | ||
protected static $defaultDescription = 'Clear the index documents'; | ||
|
||
public static function getDefaultName(): string | ||
{ | ||
return 'meilisearch:clear|meili:clear'; | ||
return self::$defaultName; | ||
} | ||
|
||
public static function getDefaultDescription(): string | ||
{ | ||
return 'Clear the index documents'; | ||
return self::$defaultDescription; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert this, as there is no point to do this at current time. When we'll be able to provide info via attributes we will.
Same in other commands
@94noni Thanks for your work. Do you have some time to finish this PR? After that PR is merged, Symfony 7 support will be easier to implement. |
@Chris53897 not so much sadly at the moment |
closing in favor of #301 which takes over |
Pull Request
Related issue
Fixes #263 (Symfony part)
What does this PR do?