Files: 
CommentFileSizeAuthor
#29 php-eval-doc-1367000-29.patch2.94 KBoriol_e9g
PASSED: [[SimpleTest]]: [MySQL] 36,882 pass(es).
[ View ]
#27 php-eval-doc-1367000-27.patch2.9 KBoriol_e9g
PASSED: [[SimpleTest]]: [MySQL] 36,924 pass(es).
[ View ]
#25 php-eval-doc-1367000-25.patch509 bytesoriol_e9g
PASSED: [[SimpleTest]]: [MySQL] 36,886 pass(es).
[ View ]
#22 php-api-docs-1367000-22.patch814 bytesDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 34,398 pass(es).
[ View ]
#9 1367000-php_clean_up_documentation-4.patch2.9 KBchris.leversuch
PASSED: [[SimpleTest]]: [MySQL] 34,357 pass(es).
[ View ]
#7 1367000-php_clean_up_documentation-3.patch1.14 KBchris.leversuch
PASSED: [[SimpleTest]]: [MySQL] 34,239 pass(es).
[ View ]
#6 1367000-php_clean_up_documentation-2.patch1016 byteschris.leversuch
PASSED: [[SimpleTest]]: [MySQL] 34,248 pass(es).
[ View ]
#2 1367000-php_clean_up_documentation-1.patch1017 byteschris.leversuch
PASSED: [[SimpleTest]]: [MySQL] 34,164 pass(es).
[ View ]
#1 1367000-php_clean_up_documentation.patch469 byteschris.leversuch
PASSED: [[SimpleTest]]: [MySQL] 34,322 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new469 bytes
PASSED: [[SimpleTest]]: [MySQL] 34,322 pass(es).
[ View ]

Think it's just a minor change needed

StatusFileSize
new1017 bytes
PASSED: [[SimpleTest]]: [MySQL] 34,164 pass(es).
[ View ]

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

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.

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

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.

Status:Needs work» Needs review
StatusFileSize
new1016 bytes
PASSED: [[SimpleTest]]: [MySQL] 34,248 pass(es).
[ View ]

Updated patch.

StatusFileSize
new1.14 KB
PASSED: [[SimpleTest]]: [MySQL] 34,239 pass(es).
[ View ]

I realised that php_eval is actually hook_filter_FILTER_process

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.

Status:Needs work» Needs review
StatusFileSize
new2.9 KB
PASSED: [[SimpleTest]]: [MySQL] 34,357 pass(es).
[ View ]

Let's try this one.

Status:Needs review» Needs work

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

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

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

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.

Status:Needs work» Needs review

Status:Needs review» Reviewed & tested by the community

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

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x.

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

time to backport!

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.

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

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

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!

Status:Needs work» Needs review
StatusFileSize
new814 bytes
PASSED: [[SimpleTest]]: [MySQL] 34,398 pass(es).
[ View ]

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?

Status:Needs review» Reviewed & tested by the community

That looks quite reasonable to me. Thanks!

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?

Assigned:chris.leversuch» Unassigned
Status:Patch (to be ported)» Needs review
StatusFileSize
new509 bytes
PASSED: [[SimpleTest]]: [MySQL] 36,886 pass(es).
[ View ]

Status:Needs review» Needs work

Ops! Not all in...

Status:Needs work» Needs review
StatusFileSize
new2.9 KB
PASSED: [[SimpleTest]]: [MySQL] 36,924 pass(es).
[ View ]

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new2.94 KB
PASSED: [[SimpleTest]]: [MySQL] 36,882 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community

Looks good to me.

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

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

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 7.x. Thanks!

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.

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.