Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix prophecies #2902

Merged
merged 5 commits into from
Dec 22, 2019
Merged

Fix prophecies #2902

merged 5 commits into from
Dec 22, 2019

Conversation

adriansuter
Copy link
Contributor

Fixes #2901.

@coveralls
Copy link

coveralls commented Dec 19, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling f4617ae on adriansuter:patch-prophecies into 4b83e02 on slimphp:4.x.

@l0gicgate
Copy link
Member

Should we update the composer file as well to 1.10?

https://github.com/slimphp/Slim/blob/4.x/composer.json#L52

@l0gicgate l0gicgate added this to the 4.4.0 milestone Dec 20, 2019
@adriansuter
Copy link
Contributor Author

Sure, I don't think anyone will get hurt.

@froschdesign
Copy link

@adriansuter @l0gicgate
First: great to see your quick reaction on this topic.
But so far it is not clear if it is a problem in Prophecy and / or a BC break. Maybe it would help if you joined the discussion to the related issue report. This could be helpful, also for others.

@adriansuter
Copy link
Contributor Author

I just wrote to the corresponding issue phpspec/prophecy#458. Let's see and wait.

In the meantime, how about updating composer.json into

        "phpspec/prophecy": ">1.8 <1.10",

@l0gicgate
Copy link
Member

l0gicgate commented Dec 20, 2019

"phpspec/prophecy": ">1.8 <1.10",

@adriansuter this is a good temporary solution.

Using Argument::is() is also a good solution but it makes me worried about all the changes we have to do to all the other repos.

Thank you for the quick PR by the way. I think we should probably wait to merge this until we hear back on phpspec/prophecy#457

@t0mmy742
Copy link
Contributor

phpspec/prophecy#460 is going to resolve this issue.

But, as mentionned in the PR, it is a best practice to use Argument::is() for strict comparison.

It is maybe better for Slim to use Argument::is() (we don't have that much changes to do in other repos).

@l0gicgate
Copy link
Member

Okay @t0mmy742 I'm going to merge this then.

@l0gicgate l0gicgate merged commit 541fad7 into slimphp:4.x Dec 22, 2019
@adriansuter adriansuter deleted the patch-prophecies branch December 26, 2019 17:14
@l0gicgate l0gicgate mentioned this pull request Jan 5, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Not an issue, just a note - tests fail
5 participants