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_

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pounard’s picture

$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.

David_Rothstein’s picture

I'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:

-      $existing_files = file_load_multiple(array(), array('uri' => $uri));
+      $existing_files = entity_load_multiple_by_properties('file', array('uri' => $uri));

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?

sun’s picture

Issue tags: +Entity system, +Novice

I 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 :)

salah1’s picture

Assigned: Unassigned » salah1
kim.pepper’s picture

Assigned: salah1 » kim.pepper
Status: Active » Needs review
FileSize
1 KB

Sorry m-abshir, I jumped in here and created a patch as it seems pretty straight forward.

Berdir’s picture

Not 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.

kim.pepper’s picture

Sorry. Got a little overexcited with the find-and-replace.

msonnabaum’s picture

The function is called entity_load_multiple_by_properties.

entity_load_multiple_by_properties($entity_type, array $properties);
kim.pepper’s picture

Thanks @msonnabaum. Makes sense.

Berdir’s picture

Hm. 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:

entity_query($entity_type)
  ->condition('name', $name)
  ->executeAndLoad();

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.

msonnabaum’s picture

Well, if redesigning it is on the table, how about we fix DX for-real?


class Entity {
  public static function findAll($properties) {
    \Drupal::service('plugin.manager.entity')
      ->getStorageController(static::type)
      ->loadByProperties($properties);
  }

  public static function type() {
    throw new RuntimeException('OH NOES');
  }
}

class Node {
  public static function type() {
    return 'node';
  }
}

Then you could just do:


$some_nodes = Node::findAll(array('name' => $name));

kim.pepper’s picture

$some_nodes = Node::findAll(array('name' => $name));

+1 with a hat-tip to rails?

msonnabaum’s picture

Sort 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.

pounard’s picture

I'd prefer a DAO pattern such as:

// Assuming we're into a MVC controller
$some_nodes = $this->getContainer()->getEntityController()->get('node')->find(array('name' => 'some_name'));

// Or could be this if the entity controller is injected
$some_nodes = $this->nodeController->find(array('name' => 'some_name'));

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.

webchick’s picture

I 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.

Berdir’s picture

Both #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.

$nodes = entity_query('node')
  ->condition('name', $name)
  ->condition('field_weight.value', 3, '>')
  ->condition('uid.entity.status', 1)
  ->sort('changed', 'DESC')
  ->executeAndLoad();

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.

pounard’s picture

Berdir++ 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:

  $this
    ->get('entity.query')
    ->get('node')
    ->condition('name', $name)
    ->condition('field_weight.value', 3, '>')
    ->condition('uid.entity.status', 1)
    ->sort('changed', 'DESC')
    ->executeAndLoad();
msonnabaum’s picture

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.

No.

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.

pounard’s picture

@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:

Drupal::getEntityController('node')->findAll(array( /* My conditions */ ));

Is not so bad for DX.

msonnabaum’s picture

You 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.

pounard’s picture

Any 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:

Drupal::entityFindAll('node', array( /* conditions */ ));

Or

Drupal::entityController('node')->findAll(array( /* conditions */ ));

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.

tim.plunkett’s picture

Did you even read #11? Node::findAll(), not on Drupal.

pounard’s picture

@#22 Did you even read #21? Hint: last paragraph.

msonnabaum’s picture

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

That's not a reason.

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).

What? Who cares. We don't need to enforce that at an API level.

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.

How exactly is it spread around? How is any contract broken?

pounard’s picture

That's not a reason.

It 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.

msonnabaum’s picture

K, your opinion is clearly stated. I disagree with nearly all of it, but lets not continue to go back and forth in this issue.

pounard’s picture

Ok, 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.

kim.pepper’s picture

Assigned: kim.pepper » Unassigned
Unitoch’s picture

Issue summary: View changes

I'm working on rerolling this patch (Austin sprint).

Unitoch’s picture

Last patch didn't apply, rerolled patch.

henk’s picture

Status: Needs review » Needs work

Patch #30 needs re-rolle, tested on latest 8.0.x.

git apply --index d8-change_entity_load_multiple_by_properties-1742850-30.patch
error: patch failed: core/includes/entity.inc:203
error: core/includes/entity.inc: patch does not apply

henk’s picture

Re-roll of patch from #30, patch is trivial. I think that discussion about Static method or procedural fucntions and design patterns are not "novice"?

henk’s picture

Status: Needs work » Needs review

#34

Patrick Storey’s picture

Issue tags: -Novice

I 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