Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Part of meta-issue #1310084: [meta] API documentation cleanup sprint
Comment | File | Size | Author |
---|---|---|---|
#29 | php-eval-doc-1367000-29.patch | 2.94 KB | oriol_e9g |
#27 | php-eval-doc-1367000-27.patch | 2.9 KB | oriol_e9g |
#25 | php-eval-doc-1367000-25.patch | 509 bytes | oriol_e9g |
#22 | php-api-docs-1367000-22.patch | 814 bytes | David_Rothstein |
#9 | 1367000-php_clean_up_documentation-4.patch | 2.9 KB | chris.leversuch |
Comments
Comment #1
chris.leversuch CreditAttribution: chris.leversuch commentedThink it's just a minor change needed
Comment #2
chris.leversuch CreditAttribution: chris.leversuch commentedMissed a word and updated _php_filter_tips. Does _php_filter_tips need @param and @return?
Comment #3
jhodgdonThanks for the patch! Good to know this module is mostly correct in doc.
Regarding:
- 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.
Comment #4
chris.leversuch CreditAttribution: chris.leversuch commentedI did consider using Implements hook_filter_FILTER_tips however the function isn't named like that - does it matter?
Comment #5
jhodgdonHm. 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.
Comment #6
chris.leversuch CreditAttribution: chris.leversuch commentedUpdated patch.
Comment #7
chris.leversuch CreditAttribution: chris.leversuch commentedI realised that php_eval is actually hook_filter_FILTER_process
Comment #8
jhodgdonThe 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.
Comment #9
chris.leversuch CreditAttribution: chris.leversuch commentedLet's try this one.
Comment #11
jhodgdonLooks like there is a D8 bad commit... don't bother to retest...
Comment #12
jhodgdonThe patch still doesn't do the php.test file.
Comment #13
chris.leversuch CreditAttribution: chris.leversuch commentedAt 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.
Comment #14
chris.leversuch CreditAttribution: chris.leversuch commentedComment #15
jhodgdonOh, sorry! I missed that. Looks like you got everything. Thanks!
Comment #16
catchCommitted/pushed to 8.x.
Comment #17
jhodgdontime to backport!
Comment #18
David_Rothstein CreditAttribution: David_Rothstein commentedHm, 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.
Comment #19
jhodgdonAh, good point. Let's leave the first sentence as it was (fixing the verb tense), and put the implements as the next paragraph.
Comment #20
chris.leversuch CreditAttribution: chris.leversuch commentedDoes the new patch need to include everything so that the initial patch can be reverted? Or just the change above?
Comment #21
jhodgdonI 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!
Comment #22
David_Rothstein CreditAttribution: David_Rothstein commentedI'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?
Comment #23
jhodgdonThat looks quite reasonable to me. Thanks!
Comment #24
catchCommitted/pushed to 8.x. I guess we can backport this?
Comment #25
oriol_e9gComment #26
oriol_e9gOps! Not all in...
Comment #27
oriol_e9gComment #28
chris.leversuch CreditAttribution: chris.leversuch commentedIt should be 'Evaluates a string of PHP code.' Other than that I think it's fine.
Comment #29
oriol_e9gComment #30
chris.leversuch CreditAttribution: chris.leversuch commentedLooks good to me.
Comment #31
xjmTook me a second to realize what was going on here. :) #29 should be a combo of #9 and #22 .
Comment #32
oriol_e9g@xjm Yes, #29 it's a combo. Includes #9 and changes in #22.
Comment #33
webchickCommitted and pushed to 7.x. Thanks!
Comment #34
David_Rothstein CreditAttribution: David_Rothstein commentedIt 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.
Comment #35
webchickOops! Thanks for catching that.
Committed and pushed to 7.x. For real this time. :) Thanks.