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

Fix PHPStan conditional return type on Search::count #2178

Merged
merged 1 commit into from
Jan 29, 2024

Conversation

Jean-Beru
Copy link
Contributor

When using Search::count method, PHPstan warns about using the result as an integer because it could also be a ResultSet.

This PR add a conditional return type depending on $fullResult param.

@ruflin
Copy link
Owner

ruflin commented Jun 30, 2023

@Jean-Beru Could you target this to 8.x and then we backport to 7.x? This ensures we always have the most recent changes in 8.x

@Jean-Beru Jean-Beru changed the base branch from 7.x to 8.x June 30, 2023 14:16
@Jean-Beru
Copy link
Contributor Author

Sure! It's done @ruflin

@ruflin
Copy link
Owner

ruflin commented Jun 30, 2023

Thanks @Jean-Beru It is ready to merge, there is only a slight problem. For some reason, Github states: "This branch has not been deployed". It seems CI did not run on this PR but it is running on other PR's like #2168 😕 @deguif Any ideas on your end? Normally I can "approve" for the first PR to run it, but it doesn't show the button 😢

@thePanz
Copy link
Collaborator

thePanz commented Jan 26, 2024

@ruflin maybe rebasing the branch again and pushing will trigger the pipelines?

The changes look good tho, we can merge this. Should we press the button? :)

@ruflin
Copy link
Owner

ruflin commented Jan 29, 2024

Thanks for the ping @thePanz I merge in main. I'm actually surprised I can push to this branch :-)

@thePanz
Copy link
Collaborator

thePanz commented Jan 29, 2024

@ruflin I guess that's because @Jean-Beru configured that the repo maintainers can push to it :)

I would suggest to "rebase" instead of merging it here: there is a configuration on the repo settings allowing to rebase from the UI directly :)

ps: I triggered the GigHub actions to run 👍

@Jean-Beru
Copy link
Contributor Author

I totally forgot this PR 😅

Thanks for the rebase @ruflin. Also, I fixed the last CI fail

@ruflin
Copy link
Owner

ruflin commented Jan 29, 2024

I would suggest to "rebase" instead of merging it here: there is a configuration on the repo settings allowing to rebase from the UI directly :)

I didn't find this button so I jumped to the command line. I normally to a rebase but felt a bit "uncomfortable" doing it on someone else code. Seems like @Jean-Beru has it now all under control :-)

@Jean-Beru
Copy link
Contributor Author

The change is trivial, I will rebase it in few minutes.

@Jean-Beru
Copy link
Contributor Author

Done 🙂

@thePanz thePanz merged commit b3840db into ruflin:8.x Jan 29, 2024
19 checks passed
@thePanz
Copy link
Collaborator

thePanz commented Jan 29, 2024

Merged, thanks @Jean-Beru !

@ruflin
Copy link
Owner

ruflin commented Jan 29, 2024

Thanks @Jean-Beru and @thePanz for getting this in!

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.

4 participants