Document the deprecated $conditions parameter in entity_load()

I could go either way on this. It is helpful to tell folks that certain code paths are being phased out, especially when the alternative is already available.

+1.

Not sure I like the comments though:

+ *   (deprecated) An array of conditions in the form 'field' => $value.
+ *   If conditions are needed, then use EntityFieldQuery to retrieve the list of
+ *   entity IDs matching a given set of conditions. Then call this function with
+ *   $ids set to the array returned by EntityFieldQuery::execute().

Can we just say something shorter like:

+ *   (deprecated) An array of conditions in the form 'field' => $value.
+ *   Use EntityFieldQuery instead to retrieve a list of IDs loadable by this function.

+1 to bojanz's fixed wording. But... these are all new functions in Drupal 7. If $conditions is actually deprecated, is it too late to just remove it?

I think it is (too late). Module porting is in full swing, no reason to kick the maintainers once again.
Of course, if anyone has proof that the $conditions param is largely unused (a grep of the CVS tree?) that would be a different story.

OK, no removal. Let's get the wording a bit better...

Unified the param description, changed the EntityFieldQuery recommendation wording.

+ *   (deprecated) An array of conditions in the form of 'field' => $value.
+ *   Use EntityFieldQuery instead to retrieve a list of entity IDs loadable by
+ *   this function.

We need to clarify that those conditions apply to the columns of the base table. The field terminology here is just confusing.

Made it match the $entity_type_load() docs.

 *   (deprecated) An array of conditions to match against the base table
 *   in the form of 'field' => $value.

I can make that "An array of conditions to match against the columns of the base table" through the code, if that's preferable.

Field is the correct term for items in a database table, not columns, so please don't change the word field to column... but I agree it's a bit confusing.

Also, please make this wrap into one paragraph:

+ *   (deprecated) An array of conditions to match against the base table
+ *   in the form of 'field' => $value.
+ *   Use EntityFieldQuery instead to retrieve a list of entity IDs loadable by
+ *   this function.

(move the sentence starting with "Use Entity..." up to the previous line and wrap to 80-character lines)

Hmmm...

What about this wording:
An associative array of conditions on the base table telling which users to load, where the keys are the database fields and the values are the values those fields must have.
(in this, "users" would be replaced by taxonomy terms etc. for the other functions)

I like the keys&values explanation (much better), I'd just kill the "telling which users to load" part, it's kinda obvious if you're reading the docs for user_load, no?

So,

An associative array of conditions on the base table, where the keys are the database fields and the values are the values those fields must have.

How does that sound?

fine with me...

One other thing I just thought of though. Maybe since it's deprectated, the next sentence should say "Instead, it is preferable to use EntityFieldQuery..." to make the "instead" more prominent?

That sounds great.
I'll do a reroll, and then we're good to go.

Missed a newline before @return in user.module

Looks OK to me. Thanks!

Maybe we should add a // @todo: remove in D8 ?

To me, "deprecated" implies 'will be removed in Drupal 8".
Did a grep through the code, seems like this is the first time we are marking something deprecated (in other cases we just use the @todo remove in D8).

I can add "@todo Remove $conditions in Drupal 8" at the end of the doc block for each function, if that would do it.

I agree that it could be implied -- I was merely suggesting to make it more explicit to be consistent.

I don't remember having any deprecated parameters but I'm OK with it. I'd also be OK with removing the parameter but that might require more discussion and investigation.

That's interesting that this is the only usage of the word "deprecated" in Drupal 7.

It's also interesting that Dries might consider removing the parameter. chx and solotandem were apparently the instigators of this issue... hopefully they can comment about removal vs. documentation.

Regarding the @todo, putting that in is probably a good idea... Back in January, sun apparently added standards for @todo blocks to the doxygen standards:
http://drupal.org/node/1354
I am not sure why, since the API module doesn't actually do anything with @todo... and I'm not sure the standards make any sense to me, but if we have standards, they should be followed.

We've had a discussion on standards for @todo by the way. Although nothing was resolved, the standards on the doxygen page remain the same:
#712876: Define coding standards for @todos

Discussed this with chx on IRC, and he's perfectly happy with just going with the docs patch (and not removing the param).
It's a small detail to argue about days before a Release Candidate, with dozen D7 books already out in the wild, referencing who knows what.

I'm a bit busy in the next few days, so if someone wants to do a reroll with the @todos before me, feel free to do so.

Rerolled with @todos.

Added a period at the of @todos. I seem to be missing that all the time :(

Looks good.

This patch seems to imply that if I went over to http://api.scratch.drupal.org/api/drupal/includes--entity.inc/class/Enti... I would find an example of how to add conditions to queries without the use of the $conditions array. However, I do not.

Is it possible to expand the PHPDoc above that section to show some @code examples so that this problem would go away?

We agreed on IRC that EntityFieldQuery can be clarified in other issues / followups, and that we shouldn't provide an EntityFieldQuery tutorial in entity load functions

Setting back to RTBC.

I think I agree with bojanz on this one. We don't provide examples in any of the other Query types. That seems more like something to put in the Handbook?

#26: 920600-26.patch queued for re-testing.

Ok, fair enough.

Committed to HEAD. Thanks!