Skip to content

Conversation

@webmozart
Copy link
Member

This PR makes search/removal in Discovery objects a bit more flexible, which is needed in practice where you may want to search bindings by query/class name.

@webmozart
Copy link
Member Author

ping @stof @tgalopin

@webmozart
Copy link
Member Author

Some more information: With this PR, things like the following are possible:

$bindings = $discovery->findBindings(
    Asset::class, 
    Expr::method('getParameterValue', 'server', Expr::same('localhost'))
);

An alternative solution is to accept a closure instead of Expression instances:

$bindings = $discovery->findBindings(
    Asset::class, 
    function (Binding $binding) { return 'localhost' === $binding->getParameterValue('server'); }
);

However, that solution forces all implementations of Discovery to load bindings into memory whenever a closure is passed. Expression instances OTOH allow, at least in theory, to be transformed into criteria (e.g. DB queries) for the backend of the implementation.

What do you think?

@stof
Copy link
Contributor

stof commented Sep 30, 2015

I agree that the expression is a better idea than the closure here, given it is for a repository class.

@stof
Copy link
Contributor

stof commented Sep 30, 2015

I haven't reviewed the implementation though

webmozart added a commit that referenced this pull request Oct 1, 2015
Added support for search/removal using Expression instances
@webmozart webmozart merged commit 2e3fa2f into master Oct 1, 2015
@webmozart webmozart deleted the expression branch August 1, 2016 13:47
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.

3 participants