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

Change casing of MeiliSearch to Meilisearch #431

Merged
merged 2 commits into from
Dec 16, 2022

Conversation

mmachatschek
Copy link
Collaborator

@mmachatschek mmachatschek commented Dec 16, 2022

Pull Request

Related issue

Fixes #<issue_number>

What does this PR do?

This PR changes the casing from MeiliSearch to Meilisearch.

To my surprise I discovered that PHP namespaces and classes are case insensitive and therefore recasing Meilisearch is possible. Therefore this change should not be breaking. I did some tests with the library and everything worked fine

See:

PR checklist

Please check if your PR fulfills the following requirements:

  • Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
  • Have you read the contributing guidelines?
  • Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!

@mmachatschek mmachatschek marked this pull request as draft December 16, 2022 08:28
@mmachatschek mmachatschek changed the title Namespace aliasing Add namespace aliasing for new Meilisearch casing Dec 16, 2022
@mmachatschek mmachatschek changed the title Add namespace aliasing for new Meilisearch casing Change casing of MeiliSearch to Meilisearch Dec 16, 2022
@mmachatschek mmachatschek marked this pull request as ready for review December 16, 2022 09:10
brunoocasali
brunoocasali previously approved these changes Dec 16, 2022
Copy link
Member

@brunoocasali brunoocasali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I share your surprise!

I made two suggestions, but in any case, is approved and thanks for that!

@brunoocasali brunoocasali added the enhancement New feature or request label Dec 16, 2022
@brunoocasali brunoocasali self-requested a review December 16, 2022 15:05
brunoocasali
brunoocasali previously approved these changes Dec 16, 2022
Copy link
Member

@brunoocasali brunoocasali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I applied the changes myself 😋

Meilisearch is made by people like you thanks a lot @mmachatschek ❤️

Copy link
Member

@brunoocasali brunoocasali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bors merge

@bors
Copy link
Contributor

bors bot commented Dec 16, 2022

@bors bors bot merged commit 19351d7 into meilisearch:main Dec 16, 2022
@mmachatschek mmachatschek deleted the namespace_aliasing branch December 16, 2022 16:33
@Nextra
Copy link
Contributor

Nextra commented Dec 27, 2022

While PHP doesn't care about case for auto loading purposes, other tools in the ecosystem will, because the specific spelling ends up in ::class strings:

> Meilisearch\Client::class
= "Meilisearch\Client"
> MeiliSearch\Client::class
= "MeiliSearch\Client"

Anything that handles these strings directly - by using the class string as an array key for example - usually cares about casing. So while this is not a breaking change in the traditional sense, users might be required to use the old spelling to interact correctly with a third party dependency.

Example: Laravel's Scout registers a pre-configured Meilisearch Client as a singleton service:
https://github.com/laravel/scout/blob/4c91081ae2dbd59b1b72a6fae2ed1b9baa8dd0d2/src/ScoutServiceProvider.php#L26-L30

Observe how using this singleton will require the old spelling:

> app()->make(Meilisearch\Client::class)
   # Illuminate\Contracts\Container\BindingResolutionException  Unresolvable dependency resolving [Parameter #0 [ <required> string $url ]] in class Meilisearch\Client.

> app()->make(MeiliSearch\Client::class)
= Meilisearch\Client {#4152}

If you keep the change, you might want to mention that this change occurred in the docs.

@brunoocasali
Copy link
Member

Hi @Nextra, this change will be appropriately handled on the Laravel side by @mmachatschek, who suggested the change :)

I've updated the release notes to include this relevant information. Thanks for sharing that!

@mmachatschek
Copy link
Collaborator Author

@brunoocasali @Nextra on laravel scouts side, yes we export the client class from the container, but this is only for convenience.

@Nextra if you rely on the client you will need to stick to the old casing (from the container). with laravel v10 this will be changed and will definitely be mentioned in the upgrade guide.

@Nextra
Copy link
Contributor

Nextra commented Jan 2, 2023

I was aware of both the PHP loading behavior and the Laravel interaction (or those of similar caching methods), so I can handle it fine. Nice to see that Scout will have a major release to match where the change can be applied and make everything consistent again. Cheers.

@mirceasoaica
Copy link

mirceasoaica commented Jan 4, 2023

This is a major change and should not happen in a minor version change.
Our production app crashed because of this change. On Mac OS both MeiliSearch and Meilisearch work, but on a linux environment they don't. Also Laravel Scout did not release an update for this change.

This should be a major version release! Not from 0.26.0 to 0.26.1.

Looks like we need to lock the meilisearch sdk version so you guys don't break it again in a minor update. This is the sort of update to release in v0.27

@mmachatschek
Copy link
Collaborator Author

@Nextra @mirceasoaica sorry for creating unexpected inconveniences on both of your sides. I created laravel/scout#687 on the scout repo to support both of the casing syntaxes hope you guys can check it out and test it as well

@jabstech
Copy link

jabstech commented Jan 5, 2023

+1, had to downgrade as well as our Linux server crashed with the same issue.
Careful for upgrading to this version as it might break your linux environment until laravel/scout#687 has been merged in.

@n8man
Copy link

n8man commented Jan 5, 2023

The changing of the namespace was handled fine by Composer, including on case-sensitive filesystems, but Composer remains case-sensitive for class names on case-sensitive filesystems... This is despite the fact that PHP is itself is case-insensitive for class names and namespaces.

This definitely should not have been part of a 0.26.0 -> 0.26.1 change. 1.0 would have been more appropriate.

@n8man
Copy link

n8man commented Jan 5, 2023

Is it reasonable at this point to keep the namespace change but revert the case change for the MeiliSearch\MeiliSearch class? I do have a PR that's mean to do that #441, but maybe that just creates more confusion.

My concern is for codebases that rely on meilisearch-php beyond laravel\scout.

bors bot added a commit that referenced this pull request Jan 10, 2023
444: Update the version for the next release (v0.27.0) r=brunoocasali a=brunoocasali

Release a new version to mark the latest changes as **breaking**.

The previous release `v0.26.1` will be deprecated in the packagist.

Related information can be verified here #441, #431

Co-authored-by: Bruno Casali <brunoocasali@gmail.com>
@brunoocasali
Copy link
Member

The naming change was intentional, but the breaking wasn't since PHP handles it for us. In any case, I was naive to assume it wouldn't break in other communities, and I'm sorry about it.

Since the change is done, the breaking was not expected and many users had already updated their applications on the laravel side for example, the most straightforward solution is to release a new version.

https://packagist.org/packages/meilisearch/meilisearch-php

I released a new version, v0.27.0, without new additions just to mark it as breaking.

@mmachatschek I think you can change in the preparation PR to use v0.27 instead.

I'm sorry again for the inconvenience.

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

Successfully merging this pull request may close these issues.

6 participants