Skip to content
This repository has been archived by the owner on Jul 3, 2020. It is now read-only.

Replace ObjectRepository with Selectable #289

Conversation

fabiocarneiro
Copy link
Contributor

findBy(['property' => []]) is a feature available in EntityManager only. Since you're typehinting ObjectRepository instead of EntityManager, is expected that this will work if you provide a MongoDB ODM DocumentManager, which unfortunately does not happen given the lack of support for values in arrays.

That said, i propose to use Selectable instead of ObjectRepository, so it will be possible to use even a Role Collection to retrieve the Roles.

This might seem very strange not to consider this a BC Break, but since you're typehinting ObjectRepository the expected behavior is already to work with any manager, and if this is not happening, i would consider this more a bugfix than a bc break. Also, the only possible working setting is EntityManager, which is a Selectable instance, so it will be perfectly compatible with any previous implementation.

@danizord
Copy link
Member

@fabiocarneiro The fact that $repository->findBy(['field_name' => ['a', 'b', 'c']]) fails is actually a limitation of the Doctrine\ODM\MongoDB\DocumentRepository implementation, not a ZfcRbac issue at all.

Having said that, I'd suggest you to keep ObjectRepositoryRoleProvider untouched and create a new SelectableRoleProvider that works with Selectable instances. Just to keep things backwards compatible.

What do you think?

@danizord danizord added the bug label Mar 12, 2015
@danizord danizord added this to the 2.5 milestone Mar 12, 2015
@danizord danizord self-assigned this Mar 12, 2015
@bakura10
Copy link
Member

You are confusing the ObjectManager and ObjectRepository interfaces here. ObjectRepository definitely have a findBy: https://github.com/doctrine/common/blob/master/lib/Doctrine/Common/Persistence/ObjectRepository.php#L64

So as @danizord said, if there is a problem in that, this must be solved on DoctrineODM, not here. We are respecting the contract ;).

@bakura10 bakura10 closed this Mar 12, 2015
@bakura10 bakura10 removed this from the 2.5 milestone Mar 12, 2015
@fabiocarneiro
Copy link
Contributor Author

@bakura10 The problem here isn't the findBy method itself, but the params provided to it. There is no contract for the value as array. We agreed that this is not a MongoDB ODM issue, but zfc-rbac, since you're relying in something you shouldn't. We've discussed it and I'll try another approach.

I updated the descriptions where possible to ObjectRepository.

@fabiocarneiro fabiocarneiro changed the title Replace ObjectManager with Selectable Replace ObjectRepository with Selectable Mar 12, 2015
@bakura10
Copy link
Member

hmmmm I see... Actually I'm not really against changing this against criteria. But rather than modifying it, I'd prefer if you could actually create a new SelectableRoleProvider that would extend ObjectRepository.

The main problem I see is that getRepository in ObjectManager returns a ObjectRepository, not a Selectable.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants