Skip to content

Conversation

@VincentLanglet
Copy link
Contributor

@VincentLanglet VincentLanglet commented Mar 14, 2023

Hi @dabeeeenster @matthewelwell

This PR solve

Deprecated: Return type of Flagsmith\Utils\Collections\FlagModelsList::jsonSerialize() should either be compatible with JsonSerializable::jsonSerialize(): mixed, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice

in a BC way

The other option would be to add : mixed return type.

@matthewelwell
Copy link
Contributor

Hi @VincentLanglet, thanks for this! What would be the impact of adding the : mixed return type instead? It seems that there is a more permanent solution needed even after either of these options, is that correct?

@VincentLanglet
Copy link
Contributor Author

Hi @VincentLanglet, thanks for this! What would be the impact of adding the : mixed return type instead? It seems that there is a more permanent solution needed even after either of these options, is that correct?

Adding a return type to a public method of a non-final method is technically a BC-break,
because if someone extends the class, it will now throw an error in the extended class, like shown in the following example
https://onlinephp.io/c/8c4da

I would say you're not supposed to extends such classes, but since they are not final you cannot be sure how they are used.
Depends on your policy, you can

  • Add returnTypeWillChange and wait for the next major to add the return type
  • Add the return type and release a new major version
  • Add the return type and release a minor version with a note about the little BC-break

@matthewelwell
Copy link
Contributor

Understood, thanks for the detailed response @VincentLanglet. Let's go with the BC option for now (as per this PR) and I will add an issue to the repository to update it.

@matthewelwell matthewelwell changed the base branch from main to release/3.1.1 March 14, 2023 16:49
@matthewelwell matthewelwell merged commit 4de24c2 into Flagsmith:release/3.1.1 Mar 14, 2023
@matthewelwell
Copy link
Contributor

Thanks again for the contribution @VincentLanglet, this is now included in v3.1.1 and I've opened a new issue to cover the full fix here.

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

Successfully merging this pull request may close these issues.

2 participants