Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chris.leversuch’s picture

Status: Active » Needs review
FileSize
469 bytes

Think it's just a minor change needed

chris.leversuch’s picture

Missed a word and updated _php_filter_tips. Does _php_filter_tips need @param and @return?

jhodgdon’s picture

Status: Needs review » Needs work

Thanks for the patch! Good to know this module is mostly correct in doc.

Regarding:

 /**
- * Tips callback for php filter.
+ * Provides 'tips callback' for php filter.
+ *
+ * @see php_filter_info
  */
 function _php_filter_tips($filter, $format, $long = FALSE) {

- First line - PHP should be all-caps, and probably should be "Provides a 'tips callback' for the PHP filter.".
- @see line -- needs () after function name.
- Regarding arguments... Actually, they are documented here:
http://api.drupal.org/api/drupal/modules--filter--filter.api.php/functio...

So probably this should just say:
Implements hook_filter_FILTER_tips().
in the first line, and then the param/return documentation can be left out.

chris.leversuch’s picture

I did consider using Implements hook_filter_FILTER_tips however the function isn't named like that - does it matter?

jhodgdon’s picture

Hm. The hook_filter_FILTER_tips() doc says that this is the "suggested" function name. I think it's OK to link. Maybe in D8 we should also consider changing the name in this core module to conform with the suggested names, but that is a different issue.

chris.leversuch’s picture

Status: Needs work » Needs review
FileSize
1016 bytes

Updated patch.

chris.leversuch’s picture

I realised that php_eval is actually hook_filter_FILTER_process

jhodgdon’s picture

Status: Needs review » Needs work

The patch is good, so far.

But I took a look at the php.module file. There are some docs lines in there that are longer than 80 characters and need to be rewrapped, which should be added to this patch.

Also, the test classes in php.test do not conform to our documentation standards, so that file needs a once-over.

chris.leversuch’s picture

Status: Needs work » Needs review
FileSize
2.9 KB

Let's try this one.

Status: Needs review » Needs work

The last submitted patch, 1367000-php_clean_up_documentation-4.patch, failed testing.

jhodgdon’s picture

Looks like there is a D8 bad commit... don't bother to retest...

jhodgdon’s picture

The patch still doesn't do the php.test file.

chris.leversuch’s picture

At the bottom of the patch there are changes for php.test e.g.

- * Base PHP test case class.
+ * Defines a base PHP test case class.

- * Create a test node with PHP code in the body.
+ * Creates a test node with PHP code in the body.

chris.leversuch’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Oh, sorry! I missed that. Looks like you got everything. Thanks!

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x.

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Status: Fixed » Patch (to be ported)
Issue tags: +Needs backport to D7

time to backport!

David_Rothstein’s picture

Version: 7.x-dev » 8.x-dev
Status: Patch (to be ported) » Needs work
- * Evaluate a string of PHP code.
+ * Implements hook_filter_FILTER_process().
....
 function php_eval($code) {

Hm, this is a little problematic. Although it's true that php_eval() is used as a filter process callback, that's not really its main purpose. Its main purpose is as an API function that other core and contrib modules use whenever they need to evaluate PHP code. I think that by changing the docs in this way we're deemphasizing that this is an API function which outside code is supposed to be calling, as an alternative to eval().

Is there a way to do both? Maybe the first sentence can be "Evaluates a string of PHP code", and somewhere further down in the docs it could mention that it also implements hook_filter_FILTER_process()?

This is an unusual situation, since it's rare for an API function to do double duty as a hook callback; I'm not sure if there are even any other examples in core.

jhodgdon’s picture

Ah, good point. Let's leave the first sentence as it was (fixing the verb tense), and put the implements as the next paragraph.

chris.leversuch’s picture

Does the new patch need to include everything so that the initial patch can be reverted? Or just the change above?

jhodgdon’s picture

I don't see a drive (or a need) to revert the original patch. So just make a new patch that fixes the identified problem. Thanks!

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
814 bytes

I'm not sure how well the "implements" line will work as the second paragraph, since it kind of interrupts the flow. All the remaining paragraphs are basically an in-depth description of how php_eval() works as an API function, so stuck in between them the "implements" seems a bit out of place.

What about putting it as the last sentence, something like the attached, maybe?

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

That looks quite reasonable to me. Thanks!

catch’s picture

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

Committed/pushed to 8.x. I guess we can backport this?

oriol_e9g’s picture

Assigned: chris.leversuch » Unassigned
Status: Patch (to be ported) » Needs review
FileSize
509 bytes
oriol_e9g’s picture

Status: Needs review » Needs work

Ops! Not all in...

oriol_e9g’s picture

Status: Needs work » Needs review
FileSize
2.9 KB
chris.leversuch’s picture

Status: Needs review » Needs work

It should be 'Evaluates a string of PHP code.' Other than that I think it's fine.

oriol_e9g’s picture

Status: Needs work » Needs review
FileSize
2.94 KB
chris.leversuch’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

xjm’s picture

Took me a second to realize what was going on here. :) #29 should be a combo of #9 and #22 .

oriol_e9g’s picture

@xjm Yes, #29 it's a combo. Includes #9 and changes in #22.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 7.x. Thanks!

David_Rothstein’s picture

Status: Fixed » Reviewed & tested by the community

It looks like this patch was committed but then rolled back. Based on the list of recently committed issues, I think the intention was to roll back some other ones instead?

I'll comment on those issues, but the patch here still applies and is still RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Oops! Thanks for catching that.

Committed and pushed to 7.x. For real this time. :) Thanks.

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