In the documentation for drupal_alter(), the parameter $data is defined as structured array, but it could also be an instance of stdClass(), for example.

Files: 
CommentFileSizeAuthor
#23 drupal-alter-docs-D6-3.patch1.62 KBrdrh555
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-alter-docs-D6-3_0.patch.
[ View ]
#18 drupal-alter-docs-D6-2.patch2.05 KBrdrh555
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-alter-docs-D6-2.patch.
[ View ]
#16 drupal-alter-docs-D6-1.patch1.84 KBrdrh555
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-alter-docs-D6-1_0.patch.
[ View ]
#11 drupal-alter-docs-D6.patch1.83 KBrdrh555
#8 917670-drupal-alter-docs-D7.patch1.8 KBmr.baileys
PASSED: [[SimpleTest]]: [MySQL] 25,298 pass(es).
[ View ]
#2 917670-drupal-alter-docs.patch889 bytesmr.baileys
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 917670-drupal-alter-docs_0.patch.
[ View ]
#1 917670-drupal-alter-docs.patch890 bytesmr.baileys
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 917670-drupal-alter-docs.patch.
[ View ]

Comments

mr.baileys’s picture

Status:Active» Needs review
StatusFileSize
new890 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 917670-drupal-alter-docs.patch.
[ View ]

Correct, drupal_alter('profile', $account); is an example where an object is passed to drupal_alter(), not an array. Patch attached for review.

mr.baileys’s picture

StatusFileSize
new889 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 917670-drupal-alter-docs_0.patch.
[ View ]

Removed a trailing whitespace.

webchick’s picture

Version:6.x-dev» 7.x-dev

We should make 6 and 7 read the same here, which means fixing this in D7 first.

D7's description atm, btw, is "&$data The primary data to be altered."

Status:Needs review» Needs work

The last submitted patch, 917670-drupal-alter-docs.patch, failed testing.

kiamlaluno’s picture

Primary data is generic enough to not exclude an object as being passed to the hook implementation.

Should we change the documentation used for Drupal 7 too, or change the documentation for Drupal 6 to describe $data as primary data?

kiamlaluno’s picture

The documentation for Drupal 7 cannot be the same for Drupal 6, though.

In Drupal 6, drupal_alter() uses only a parameter passed by reference, while in Drupal 7 it uses three parameters that are passed by reference; calling $data the primary data makes sense in that base.
The documentation for Drupal 6 should also report the trick of using an array containing the index '__drupal_alter_by_ref' to pass two by-reference arguments to the hook implementation. Right now, there is just a comment in the code that reports how to pass two arguments by reference to the hooks.

mr.baileys’s picture

Status:Needs work» Needs review

@webchick: I'd left the D7 version alone since contrary to the D6 version, it's not buggy. I do agree it makes sense to reword it so the D6 and D7 versions are more similar.
@kiamlaluno: good point, I'll address that once the D7 patch is in and we're back to D6.

D7 patch attached, made some additional (minor) edits:

  • "A string describing the data type of the alterable $data." => slightly altered to read "A string describing the type of the alterable $data.", as "data type" is misleading (we are not talking int, string and array here, but things like "form", "profile", "user", etc.
  • "…this should be a keyed array as described above…" => changed to use "associative array" as this is more widely used in core docs.
mr.baileys’s picture

StatusFileSize
new1.8 KB
PASSED: [[SimpleTest]]: [MySQL] 25,298 pass(es).
[ View ]

Dear d.o., stop eating my patches...

kiamlaluno’s picture

Status:Needs review» Reviewed & tested by the community

The patch is good, IMO; it makes the description for Drupal 6 and 7 more similar of what they are now.

Dries’s picture

Version:7.x-dev» 6.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

Committed to CVS HEAD. Moving to Drupal 6.

rdrh555’s picture

Assigned:Unassigned» rdrh555
Status:Patch (to be ported)» Needs review
StatusFileSize
new1.83 KB

Patch for D6.

jhodgdon’s picture

Status:Needs review» Needs work

I am not too happy with this:

  * @param ...
- *   Any additional params will be passed on to the called
- *   hook_$type_alter functions.
+ *   In case more arguments need to be passed and alterable, modules may provide
+ *   additional variables by using an array containing the index
+ *   '__drupal_alter_by_ref' to pass two by-reference arguments to the
+ *   hook implementation.

I think the wording is very confusing, and it doesn't explain how to use the __drupal_alter_by_ref or how to use this in general. Also, this seems to imply that all ... args must be alterable, which isn't the case. And the alter_by_ref thing doesn't make sense to me, and implies there can only be two by-reference args -- I'm not seeing that in the code.

rdrh555’s picture

Yeah, struggled w/that ...how about this?
@param ....
drupal_alter() normally supports just one by reference parameter. Additional params that need to be passed by reference should be stored within the $data array using the key '__drupal_alter_by_ref' . They will then be passed on to the called hook_$type_alter functions.

jhodgdon’s picture

The @param for ... needs to say that it's for additional arguments that are not passed by reference, which is still not mentioned in your new proposal.

Probably the stuff about __drupal_alter_by_ref should go in the @param for $data, since that's where you pass it in?

rdrh555’s picture

Status:Needs work» Needs review

Agreed.

rdrh555’s picture

StatusFileSize
new1.84 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-alter-docs-D6-1_0.patch.
[ View ]

Revised patch.

jhodgdon’s picture

Status:Needs review» Needs work

OK, this is much better.

The only little thing I would change now is that some places, it says hook_TYPE_alter() and some places, it says hook_$type_alter. I think the first form is better?

Hmmm...
" Additional params that need to be passed by reference should be stored using an array with the key '__drupal_alter_by_ref'."

Maybe this would be better as:

If you need to pass additional parameters by reference to the hook_TYPE_alter() functions, include them as an array in $data['__drupal_alter_by_ref']. They will be unpacked and passed to the hook_TYPE_alter() functions, before the additional ... parameters (see below).

Also, I think we should say "parameters" not "params" in the "..." section. How about:

Any additional parameters will be passed on to the hook_TYPE_alter() functions (not by reference), after any by-reference parameters included in $data (see above).

Thoughts?

rdrh555’s picture

Status:Needs work» Needs review
StatusFileSize
new2.05 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-alter-docs-D6-2.patch.
[ View ]

Thoughts are: consistency is key, and, by merging in proposed changes, this patch now clearly communicates the handling of the different(additional) parameters. Thank you for the suggestions.

Status:Needs review» Needs work

The last submitted patch, drupal-alter-docs-D6-2.patch, failed testing.

jhodgdon’s picture

Status:Needs work» Reviewed & tested by the community

This is fine. Ignore the "test" failure - the bot is not working right now for D6.
Thanks!

Gábor Hojtsy’s picture

Status:Reviewed & tested by the community» Needs work

Hum, it does not look like that $type can be an array.

jhodgdon’s picture

You are right -- in D6 $type cannot be an array. Sorry for not noticing that in my review -- I must have been looking at the D7 code by mistake. That does need to be removed from the patch.

rdrh555’s picture

Status:Needs work» Needs review
StatusFileSize
new1.62 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-alter-docs-D6-3_0.patch.
[ View ]
new1.62 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-alter-docs-D6-3.patch.
[ View ]

New patch per Gabor's comment:

rdrh555’s picture

augh! d.o. grabbed it twice somehow and I don't even know if there is a way to "unsend", sorry!

Status:Needs review» Needs work

The last submitted patch, drupal-alter-docs-D6-3.patch, failed testing.

jhodgdon’s picture

Status:Needs work» Reviewed & tested by the community

Thanks, looks good! And ignore it if the bot says it doesn't apply - the patch is fine.

Gábor Hojtsy’s picture

Status:Reviewed & tested by the community» Fixed

Great, thanks committed.

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.