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

Allow doctrine/collections:^2.0 as alternative dependency #187

Merged
merged 1 commit into from
Feb 14, 2023

Conversation

toniperic
Copy link
Contributor

Given this, seems no harm to include major version 2 as an alternative dependency.

I only noticed this as one of my projects wanted to upgrade to doctrine/collections:^2.0 but couldn't because liuggio/fastest depends exclusively on major version 1.

@janklan
Copy link

janklan commented Feb 13, 2023

Yes Please!

@DonCallisto DonCallisto merged commit fec82d7 into liuggio:master Feb 14, 2023
@DonCallisto
Copy link
Collaborator

@mnocon
Copy link
Contributor

mnocon commented Mar 2, 2023

We have the same failure in our CI pipeline:

PHP Fatal error:  Uncaught TypeError: Liuggio\Fastest\Queue\TestsQueue::add(): Return value must be of type bool, null returned in /var/www/vendor/liuggio/fastest/src/Queue/TestsQueue.php:56
Stack trace:
#0 /var/www/vendor/liuggio/fastest/src/Queue/CreateTestsQueueFromSTDIN.php(51): Liuggio\Fastest\Queue\TestsQueue->add()
#1 /var/www/vendor/liuggio/fastest/src/Queue/CreateTestsQueueFromSTDIN.php(32): Liuggio\Fastest\Queue\CreateTestsQueueFromSTDIN->addLineIfNotEmpty()
#2 /var/www/vendor/liuggio/fastest/src/Queue/ReadFromInputAndPushIntoTheQueue.php(28): Liuggio\Fastest\Queue\CreateTestsQueueFromSTDIN->execute()
#3 /var/www/vendor/liuggio/fastest/src/Command/ParallelCommand.php(101): Liuggio\Fastest\Queue\ReadFromInputAndPushIntoTheQueue->execute()

The v1 of Collections used to return true after every add execution:
https://github.com/doctrine/collections/blob/1.8.x/lib/Doctrine/Common/Collections/ArrayCollection.php#L313-L326

But now it doesn't return anything:
https://github.com/doctrine/collections/blob/2.1.x/src/ArrayCollection.php#L326-L329

I can prepare a PR to this repository, changing the return value of add to true:
https://github.com/liuggio/fastest/blob/master/src/Queue/TestsQueue.php#L45-L57

to keep BC.

Is that ok or do you have another suggestion?

@DonCallisto
Copy link
Collaborator

If I'm not mistaken we never return that boolean value.
It's true that a change in return type could be a BC but only if someone explicitly extend from that class.

Possible solutions:

  • drop the bool return type and release a new major version
  • trigger a deprecation warning, return true and drop the bool return value in next major
  • keep the bool value without emitting a warning

WDYT?

@mnocon
Copy link
Contributor

mnocon commented Mar 6, 2023

I don't think this small change justifies releasing a new major.

I would go for option 2 (trigger a deprecation warning, return true and drop the bool return value in next major), I feel it's the best of two worlds - we keep the v1 working and are ready to make changes for v2 in the future.

I can prepare a PR with the changes if you agree

@DonCallisto
Copy link
Collaborator

Yes please, go on with option 2

@mnocon
Copy link
Contributor

mnocon commented Mar 7, 2023

I've created a PR: #188

@DonCallisto
Copy link
Collaborator

Resolved with #188

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