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

MailboxIterface: Typehint to interface instead of implementation #471

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

homersimpsons
Copy link

Prefer MessageIteratorInterface instead of MessageIterator.

This should not be a breaking change as MessageIterator implements MessageIteratorInterface

Prefer `MessageIteratorInterface` instead of `MessageIterator`.

This should not be a breaking change as `MessageIterator implements MessageIteratorInterface`
@Slamdunk
Copy link
Collaborator

Nice, could you also update Mailbox as well please?

Reflect changes to MailboxInterface
@homersimpsons
Copy link
Author

homersimpsons commented Jul 28, 2020

Thank you for this blazing fast response.

I did update it.

The problem is that Mailbox::prepareMessageIds needs \ArrayIterator::getArrayCopy which is not in MailboxInterface. What is the expected fix ?

@Slamdunk
Copy link
Collaborator

The issue is that ArrayIterator::getArrayCopy is not part of any interface.

Now I remember this is the reason why I type-hinted against MessageIterator and not MessageIteratorInteface.

I see no solution to implement any part of this PR.

@homersimpsons
Copy link
Author

Actually we could have something along

if ($messageIds instanceof MessageIterator) {
    $messageIds = $messageIds->getArrayCopy();
} elseif ($messageIds instanceof MessageIteratorInterface) {
    $messageIds = iterator_to_array($messageIds, false);
}

@Slamdunk
Copy link
Collaborator

That wouldn't respect proper type covariance in type hints.

We should set MessageIteratorInterface to extend Traversable interface, to we can easily use iterator_to_array as you suggest

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