There's two things which didn't sit 100% right with entity_load_multiple_by_properties():
+ * @param array $values
+ * An associative array of properties of the entity, where the keys are the
+ * property names and the values are the values those properties must have.
...
+function entity_load_multiple_by_properties($entity_type, array $values) {
webchick didn't like the name of the $values array, but berdir correctly pointed out that it's an array of values rather than an array of properties (properties are the keys). This used to be $conditions which sort of encompasses both and we could maybe go back to that, but I don't think it's worth holding the patch over since we're not sure yet.
Also we could look at adding a single version of this, to replace one-off helpers like entity_load_by_name()
Also we could consider adding a single version of this, to try to replace one-off helpers like user_load_y_
Comment | File | Size | Author |
---|---|---|---|
#34 | d8-change_entity_load_multiple_by_properties-1742850-34.patch | 896 bytes | henk |
#30 | d8-change_entity_load_multiple_by_properties-1742850-30.patch | 915 bytes | Unitoch |
#9 | 1742850-values-to-properties-9.patch | 949 bytes | kim.pepper |
#7 | 1742850-7-values-to-conditions.patch | 964 bytes | kim.pepper |
#5 | 1742850-values-to-conditions.patch | 1 KB | kim.pepper |
Comments
Comment #1
pounard$values is fine for me, Berdir is right!
It doesn't make sense to have a single version of that, the _multiple_ infix makes no sense in the name, the only non multiple loader we will ever have is the myentity_load_by_id(), all others cannot guarantee a single result, it would be semantically false, at least if the API remains abstracted of the entity type.
Comment #2
David_Rothstein CreditAttribution: David_Rothstein commentedI'm not sure if this is the right issue for this, but to the extent it's intended to be for catch-all followup to the introduction of entity_load_multiple_by_properties(), I'll point it out here for starters:
That change (and others like it from the patch committed in the other issue) introduces a big API inconsistency. Why isn't it
file_load_multiple_by_properties(array('uri' => $uri))
instead?Comment #3
sunI agree that $conditions makes more sense than $values or $properties. Let's do that.
@David_Rothstein: Ideally, I think we want to get rid of all the $module_load_* helper/wrapper functions in favor of only using the entity_* functions for D8, because the helpers are a bit silly to have in the first place, pure artifacts of the past.
The suggested changes sound pretty easy to me, so let's try whether a novice can take this up :)
Comment #4
salah1Comment #5
kim.pepperSorry m-abshir, I jumped in here and created a patch as it seems pretty straight forward.
Comment #6
BerdirNot sure if this makes sense, at least not the changed documentation. It is talking about array keys and values, not the argument names values so it shouldn't be changed.
Comment #7
kim.pepperSorry. Got a little overexcited with the find-and-replace.
Comment #8
msonnabaum CreditAttribution: msonnabaum commentedThe function is called entity_load_multiple_by_properties.
Comment #9
kim.pepperThanks @msonnabaum. Makes sense.
Comment #10
BerdirHm. We're actually using "entity fields" now instead of "entity properties", and instead talk about configurable (field.module) and non-configurable (provided by node.module for node entity) fields.
The entity query API was improved *a lot* since this was added. I think if we would add something like executeAndLoad() (or a fancy lazy-load collection thingy but such a method would be easier for now), then we could almost consider to remove it favor of using something like:
While that's 3 instead of a single line, it is, I think, also more readable and a lot easier to extend as soon as you have slightly more complicated conditions, multiple conditions or want to use something like range/sort.
Comment #11
msonnabaum CreditAttribution: msonnabaum commentedWell, if redesigning it is on the table, how about we fix DX for-real?
Then you could just do:
Comment #12
kim.pepper+1 with a hat-tip to rails?
Comment #13
msonnabaum CreditAttribution: msonnabaum commentedSort of. You'll find that pattern in any project uses the activerecord or datamapper pattern, which is nearly everyone but us. Doctrine looks very similar as well.
Comment #14
pounardI'd prefer a DAO pattern such as:
Which is in the end more or less the same thing, you just move the finder onto a service object - which we already have known as the entity controllers- or we could just unify the EFQ to support all of the dynamic and "property" fields.
Berdir++ in #10.
Comment #15
webchickI am officially rage-quitting Drupal if we force people to write code like #14 to get a list of nodes. :P~
#11 fills me less with rage.
Comment #16
BerdirBoth #11 and #14 are a refactoring of the method. That's not what I was suggesting. I was suggesting to remove it in favor of the now awesome EFQ API, which already *did* unify configurable and non-configurable fields, supports relationships, aggregations and what not.
@webchick: There should be a lot less reasons to write code to get a list of nodes now with Views in Core :) And my argument is that a list of nodes is very seldomly so easy as just based on a single name or type condition. Probably you also want that sorted based on the changed date and only if your custom weight field is higher than 3 and it's written by a user with status 1.
So, my argument is that you only need to learn a single API and not have to rewrite your code as soon as you get a slightly more complex use case.
Comment #17
pounardBerdir++ EFQ++
@#15 If you don't like #14, it's actually what Drupal would need you to do right now, since the EFQ object will be given by some factory into the DIC. Actually in order to use EFQ in a ContainerAware object you should:
Comment #18
msonnabaum CreditAttribution: msonnabaum commentedNo.
I've needed this functionality many times in Drupal projects, where I didn't need sorting or anything else. This functionality exists in many ORMs/frameworks:
http://docs.doctrine-project.org/en/2.0.x/reference/working-with-objects...
http://backbonejs.org/#Collection-findWhere
http://api.rubyonrails.org/classes/ActiveRecord/FinderMethods.html#metho...
That's what this function already does. We're talking about replacing a procedural function, so we should try to match the DX that that provided. Anyone within a class is totally welcome to inject their entity query object and go to town, but that doesn't mean we need to ruin DX for everyone else.
Comment #19
pounard@msonnabaum
The future of Drupal from the moment core dev choose to use DIC is never going out of an object scope anymore, and use what the context provides, not a global scoped helper. I'm not against some helpers such as findAll() findOne() etc... but they must be called on an object, not globally. This means that whatever the context is, you always need to have at least your entity controller to be injected.
For procedural code, having:
Is not so bad for DX.
Comment #20
msonnabaum CreditAttribution: msonnabaum commentedYou don't need to explain to me how the DIC works.
If we're offering access via a static method call, why not make it simple and not expose concepts like "entity controllers"? It just makes the call more complex for no gain at all.
Comment #21
pounardAny findX() implementation will rely on either EFQ or be specific to the storage backend. If you don't expose the entity controller you at least need to expose the entity type into your method signature. The bare minimum would then be:
Or
This is a matter of taste, but I think exposing the entityController sounds less circumvoluted, if you start to expose every helper method on the Drupal class, autocompletion will be like trying to read a complete french dictionnary with no index and DX would be thousand times worst.
If we want procedural helpers, I think that having static methods on the Node class, for example, is both wrong and dangerous: wrong because it's against the design and it taints the OOP code and spreads the static/global code into components where it shouldn't live, and dangerous because nothing prevent a module from changing the node storage and controller, and node class (even thought it would easy to call Drupal::something() in Node::findAll() but this is an awful lot of code indirection). Moving static helpers arround the OOP components breaks the interface design and contract and makes it harder to maintain because it spreads static code all arround where this layer should only exist as a compatibility layer, and be isolated from everything else.
Comment #22
tim.plunkettDid you even read #11? Node::findAll(), not on Drupal.
Comment #23
pounard@#22 Did you even read #21? Hint: last paragraph.
Comment #24
msonnabaum CreditAttribution: msonnabaum commentedThat's not a reason.
What? Who cares. We don't need to enforce that at an API level.
How exactly is it spread around? How is any contract broken?
Comment #25
pounardIt is actually a very good reason, in my opinion, but it is also valid to think not. Let me explain my opinion.
--
It does not literally break the interface contract, it just gives more responsability to the component. The more we'll spread code arround in those components, unrelated to the contract itself, the less the code will be readable.
We'll also give more than one way to do the exact same thing, and confuse users.
Drupal 8 chose the Symfony way, we have to assume it. Providing yet another round/layer of procedural helper is against helping people migrate to the new design. The more of those helper we'll provide, the more modules we'll see not "doing it right".
If you want to play cross-words, yes, I misued "procedural", but writing static methods is just like writing procedural helpers, doesn't really make any difference outside of the namespacing.
The
Drupal
class PHPdoc states: This class exists only to support legacy code that cannot be dependency injected. If your code needs it, consider refactoring it to be object oriented, if possible. There is no reason we start now going against this. Today the OOP API provide way more helpers than Drupal 7 was providing, in the form of the EFQ and all, which are directly get-able for the procedural code via the Drupal class.By centralizing everything into the Drupal class, it will also make it easier to people for refactoring their code when all those static helpers will be removed in maybe Drupal 9. Because yes, they will probably die (or be renamed, or just be moved out), giving a single point of entry for those static helpers make it easier to grep and fix.
Going backward now is making the big picture totally inconsistent. Drupal 7 has is whole lot of garbage helpers, inconsistent APIs and @todos already, I'd be quite happy to see Drupal 8 not having the same thing.
If we take a step back and look to all of this, there is no way you, as a core developer, would code anything "the old ways" and not using the Drupal 8 API, and there is no reason we should encourage people to do so either.
And finally, considering what showed Berdir, I think the API we have now is already easy to use enough. Your find() and all helpers are indeed a good idea, I just don't see them fiting anywhere else as a legit part of the API onto the controllers themselves, because it's an API that wouldn't just be good for procedural code, but also for OOP code having it (the controller) injected magically.
Of course, that's just an opinion.
Comment #26
msonnabaum CreditAttribution: msonnabaum commentedK, your opinion is clearly stated. I disagree with nearly all of it, but lets not continue to go back and forth in this issue.
Comment #27
pounardOk, yours is legit too. Your original idea of providing such method helper is definitely (IMO) a good idea, I just disagree where those should be.
Considering that, I'm all in for a compromise, and I think that
Drupal::entityController('node')->find(array( /* */ ))
is not a bad one.I leave this to other people's opinion.
Comment #28
kim.pepperComment #29
Unitoch CreditAttribution: Unitoch commentedI'm working on rerolling this patch (Austin sprint).
Comment #30
Unitoch CreditAttribution: Unitoch commentedLast patch didn't apply, rerolled patch.
Comment #32
henk CreditAttribution: henk commentedPatch #30 needs re-rolle, tested on latest 8.0.x.
Comment #34
henk CreditAttribution: henk commentedRe-roll of patch from #30, patch is trivial. I think that discussion about Static method or procedural fucntions and design patterns are not "novice"?
Comment #35
henk CreditAttribution: henk commented#34
Comment #36
Patrick Storey CreditAttribution: Patrick Storey at Hook 42 commentedI am removing the Novice tag from this issue because it was added in 2012 back when there was a clear direction but as @henk pointed out, this has become a discussion on design patters and such matters are not something we want new contributes tackling as one of their first issues.
I’m using this documentation as a source: https://www.drupal.org/core-mentoring/novice-tasks#avoid
Comment #37
wizonesolutionsGiven that there are issues like #1887058: Remove usages of entity_load_multiple_by_properties() and EntityStorageController::loadByProperties() and #1903774: Replace entity_load_multiple_by_properties() uses in menu module/menu tests with an entity_query, I'm pretty sure this is a wontfix now.