CommentFileSizeAuthor
#76 filter_docs-1347914-76.patch38.16 KBAlbert Volkman
#73 filter_docs-1347914-73.patch18.5 KBAlbert Volkman
#73 interdiff.txt829 bytesAlbert Volkman
#71 filter_docs-1347914-71.patch18.5 KBAlbert Volkman
#71 interdiff.txt4.69 KBAlbert Volkman
#69 1347914-69-filter-docs.patch23.08 KBLars Toomre
#66 doc-cleanup-filter-module-1347914-66.patch2.15 KBbatigolix
#64 doc-cleanup-filter-module-1347914-64.patch2.59 KBbatigolix
#60 doc-cleanup-filter-module-1347914-60.patch37.56 KBbatigolix
#53 doc-cleanup-filter-module-1347914-53.patch37.88 KBbatigolix
#50 doc-cleanup-filter-module-1347914-50.patch41.64 KBNROTC_Webmaster
#50 filter-interdiff.txt2.87 KBNROTC_Webmaster
#42 doc-cleanup-filter-module-1347914-42.patch39.65 KBNROTC_Webmaster
#42 filter-interdiff.txt10.13 KBNROTC_Webmaster
#40 doc-cleanup-filter-module-1347914-40.patch38.72 KBNROTC_Webmaster
#40 filter-interdiff.txt8.92 KBNROTC_Webmaster
#37 doc-cleanup-filter-module-1347914-37.patch40.77 KBNROTC_Webmaster
#37 filter-interdiff.txt25.01 KBNROTC_Webmaster
#35 doc-cleanup-filter-module-1347914-35.patch41.22 KBNROTC_Webmaster
#35 filter-interdiff.txt25.47 KBNROTC_Webmaster
#32 doc-cleanup-filter-module-1347914-32.patch34.5 KBNROTC_Webmaster
#30 doc-cleanup-filter-module-1347914-30.patch34.81 KBNROTC_Webmaster
#28 doc-cleanup-filter-module-1347914-28.patch34.81 KBNROTC_Webmaster
#28 filter-interdiff.txt3.9 KBNROTC_Webmaster
#23 doc-cleanup-filter-module-1347914-23.patch34.8 KBNROTC_Webmaster
#21 doc-cleanup-filter-module-1347914-21.patch35.1 KBNROTC_Webmaster
#21 filter-interdiff.txt32.44 KBNROTC_Webmaster
#7 doc-cleanup-filter-module-1347914-7.patch33.29 KBNROTC_Webmaster
#5 doc-cleanup-filter-module-1347914-5.patch33.49 KBNROTC_Webmaster
#3 doc-cleanup-filter-module-1347914-partial.patch22.52 KBsven.lauer
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

Issue tags: +Novice, +Needs backport to D7

@sven.lauer: Do you still plan to work on this? If so, please respond. If not, we'll unassing so someone else can. Thanks!

Also, tagging.

sven.lauer’s picture

Duh, I kinda half-forgot that I was working on this. I'll post a first patch soon (or at least my partial patch and unassign). Sorry about that.

sven.lauer’s picture

Assigned: sven.lauer » Unassigned
FileSize
22.52 KB

Okay, it seems like I don't have the time to do this soon, so unassigning.

I attach a partial patch. From looking at it, I think it is only (part of) filter.module that needs to be done, but no guarantees.

(The patch applies against the current 8.x branch.)

xjm’s picture

Status: Active » Needs work

Marking NW since there's a partial patch. Thanks @sven.lauer!

NROTC_Webmaster’s picture

Status: Needs work » Needs review
FileSize
33.49 KB

Hopefully I didn't miss too much on this one but it is a big project compared to what I have been working on.

Status: Needs review » Needs work

The last submitted patch, doc-cleanup-filter-module-1347914-5.patch, failed testing.

NROTC_Webmaster’s picture

Status: Needs work » Needs review
FileSize
33.29 KB
jhodgdon’s picture

jhodgdon’s picture

Sorry this got stuck at "needs review". Let's see if the test bot is still happy with it, then get it reviewed and committed!

Status: Needs review » Needs work
Issue tags: -Novice, -Needs backport to D7, -docs-cleanup-2011

The last submitted patch, doc-cleanup-filter-module-1347914-7.patch, failed testing.

jhodgdon’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, doc-cleanup-filter-module-1347914-7.patch, failed testing.

jhodgdon’s picture

Status: Needs work » Needs review
Issue tags: +Novice, +Needs backport to D7, +docs-cleanup-2011
xjm’s picture

Status: Needs review » Needs work

Got halfway through my review and hit the wrong button in dreditor, but here's what I had up to that point:

  1. +++ b/core/modules/filter/filter.admin.incundefined
    @@ -1,15 +1,15 @@
     <?php
    -
    
    +++ b/core/modules/filter/filter.api.phpundefined
    @@ -1,5 +1,4 @@
     <?php
    -
     /**
    
    +++ b/core/modules/filter/filter.moduleundefined
    @@ -1,8 +1,7 @@
     <?php
    -
    
    +++ b/core/modules/filter/filter.testundefined
    @@ -1,12 +1,11 @@
     <?php
    -
     /**
    

    This blank line is there in all core files, as far as I know, so let's not remove it? (And add it in the other files.)

  2. +++ b/core/modules/filter/filter.admin.incundefined
    @@ -1,15 +1,15 @@
    - * Menu callback; Displays a list of all text formats and allows them to be rearranged.
    + * Form constructor for listing and rearranging text formats.
    

    How about:
    Form constructor for a form to list and reorder text formats.

  3. +++ b/core/modules/filter/filter.moduleundefined
    @@ -164,29 +167,32 @@ function filter_format_load($format_id) {
    + * @return
    + *   SAVED_NEW or SAVED_UPDATED
    

    Period needed here, I think.

  4. +++ b/core/modules/filter/filter.moduleundefined
    @@ -931,7 +954,7 @@ function filter_process_format($element) {
    - * #pre_render callback for #type 'text_format' to hide field value from prying eyes.
    + * #pre_render callback: Hide field value of 'text_format' elements.
    

    See http://drupal.org/node/1354#render for how to document this guy.

  5. +++ b/core/modules/filter/filter.moduleundefined
    @@ -996,7 +1019,24 @@ function filter_access($format, $account = NULL) {
    + * @param $long
    + *   Boolean indicating whether the long form of tips should be returned.
    

    This parameter is optional, correct?

  6. +++ b/core/modules/filter/filter.moduleundefined
    @@ -1247,7 +1288,7 @@ function _filter_html($text, $filter) {
    - * Filter tips callback for HTML filter.
    + * Filters the tips callback for the HTML filter.
    
    @@ -1345,7 +1386,7 @@ function _filter_html_tips($filter, $format, $long = FALSE) {
    - * Settings callback for URL filter.
    + * Settings callback for the URL filter.
    
    @@ -1525,7 +1566,7 @@ function _filter_url_parse_partial_links($match) {
    - * preg_replace callback to escape contents of HTML comments
    + * preg_replace callback to escape contents of HTML comments.
    
    @@ -1579,21 +1620,21 @@ function _filter_url_trim($text, $length = NULL) {
    - * Filter tips callback for URL filter.
    
    @@ -1659,7 +1700,7 @@ function _filter_autop($text) {
    - * Filter tips callback for auto-paragraph filter.
    + * Filters the tips callback for the auto-paragraph filter.
    
    @@ -1678,7 +1719,7 @@ function _filter_html_escape($text) {
    - * Filter tips callback for HTML escaping filter.
    + * Filters the tips callback for an HTML escaping filter.
    

    These need some work. In particular I think "Filters the tips callback" is incorrect. They're (apparently) callbacks for filter tips, not things filtering "tips callbacks."

  7. +++ b/core/modules/filter/filter.testundefined
    @@ -184,7 +183,9 @@ class FilterAdminTestCase extends DrupalWebTestCase {
       }
    -
    +  /**
    +   * Tests the text format adminstration functionality.
    +   */
    

    I think there needs to be a blank line here as well.

  8. +++ b/core/modules/filter/filter.testundefined
    @@ -1713,7 +1726,10 @@ body {color:red}
    +   * Asserts that a normalized string contains a given other string.
    

    "A given other string" seems awkward (or maybe it's just me?) Maybe "another provided string"?

  9. +++ b/core/modules/filter/filter.testundefined
    @@ -1713,7 +1726,10 @@ body {color:red}
    +   * Normalization here means transforming the string to lowercase and decoding
    +   * HTML entities.
    

    Hmm, I think this is a bit awkward. Maybe lose this paragraph and make the summary say something like:
    Asserts that a lowercased, decoded HTML string contains another.
    (or something like that?) This actually would fix the previous point (and it applies to the next change in the patch as well).

xjm’s picture

OK I guess I was almost through the patch. One other thing:

+++ b/core/modules/filter/filter.testundefined
@@ -1048,7 +1061,7 @@ class FilterUnitTestCase extends DrupalUnitTestCase {
-   * Test filter settings, defaults, access restrictions and similar.
+   * Tests filter settings, defaults, access restrictions, and similar things.

Maybe just "etc." rather than "and similar things"? :)

I didn't apply the patch locally to check for missed bits; the above points are just from reading the patch.

NROTC_Webmaster’s picture

xjm,

For #4 where the documentation says "- Somewhere in the function, there should be a paragraph saying where the callback is assigned and what particular Render API callback it is being used for."
Since the function itself never gives any other details should I add that it is a #pre_render callback in the long description?

For the first comment in #6 would this be appropriate? Also I'm not sure what to put for the within. It is called from filter_filter_info() and hook_filter_info() but neither of those really seem to follow the example of uasort() within system_themes_page().
* Provides tips based on the HTML filter.
*
* Callback for _filter_tips() within system_themes_page().

xjm’s picture

For #4 where the documentation says "- Somewhere in the function, there should be a paragraph saying where the callback is assigned and what particular Render API callback it is being used for."
Since the function itself never gives any other details should I add that it is a #pre_render callback in the long description?

This seems correct to me. I'd also suggest grepping core for how this callback is used (grep the function name without parens) and see if that is worth documenting.

Checking on the other thing now.

xjm’s picture

For the first comment in #6 would this be appropriate? Also I'm not sure what to put for the within. It is called from filter_filter_info() and hook_filter_info() but neither of those really seem to follow the example of uasort() within system_themes_page().
* Provides tips based on the HTML filter.

So whaddya know, there is such a thing as a "tips callback." (This is one of the defined keys for hook_filter_info(), in a similar vein to hook_menu().) It's not really "based on" the HTML filter though--it's intended for it. Maybe something along the lines of:

/**
 * Filter tips callback: Provides filter tips for the HTML filter.
 *
 * @see filter_filter_info()
 */

You'd want to look each one up to check the specifics.

NROTC_Webmaster’s picture

Below is how I modified the following functions with the new way below each. There are two of them that exceed 80 characters but I'm not sure what to do in this case because the callback takes up almost 40 of the 80 characters.

@@ -1247,7 +1288,7 @@ function _filter_html($text, $filter) {
- * Filter tips callback for HTML filter.
+ * Filters the tips callback for the HTML filter.
/**
 * Filter tips callback: Provides filter tips for the HTML filter.
 *
 * @see filter_filter_info()
 */

@@ -1345,7 +1386,7 @@ function _filter_html_tips($filter, $format, $long = FALSE) {
- * Settings callback for URL filter.
+ * Settings callback for the URL filter.
/**
 * Filter URL Settings callback: Provides settings for the URL filter.
 *
 * @see filter_filter_info()
 */

@@ -1525,7 +1566,7 @@ function _filter_url_parse_partial_links($match) {
- * preg_replace callback to escape contents of HTML comments
+ * preg_replace callback to escape contents of HTML comments.
/**
 * Preg replace callback: Escapes the contents of HTML comments.
 *
 * @param $match
 *   An array containing matches to replace from preg_replace_callback(),
 *   whereas $match[1] is expected to contain the content to be filtered.
 * @param $escape
 *   (optional) Boolean whether to escape (TRUE) or unescape comments (FALSE).
 *   Defaults to neither. If TRUE, statically cached $comments are reset.
 *
 * @see preg_replace_callback.
 */

@@ -1579,21 +1620,21 @@ function _filter_url_trim($text, $length = NULL) {
- * Filter tips callback for URL filter.
+ * Filters the tips callback for the URL filter.
/**
 * Filter URL tips callback: Provides filter tips for the URL filter.
 *
 * @see filter_filter_info()
 */

@@ -1659,7 +1700,7 @@ function _filter_autop($text) {
- * Filter tips callback for auto-paragraph filter.
+ * Filters the tips callback for the auto-paragraph filter.
/**
 * Filter autop tips callback: Provides filter tips for the auto-paragraph filter.
 *
 * @see filter_filter_info()
 */

@@ -1678,7 +1719,7 @@ function _filter_html_escape($text) {
- * Filter tips callback for HTML escaping filter.
+ * Filters the tips callback for an HTML escaping filter.
/**
 * Filter HTML escape tips callback: Provides filter tips for an HTML escaping filter.
 *
 * @see filter_filter_info()
 */

Additonally since there are three other callbacks to preg_replace should I update those as well such as.

 * preg_replace callback to make links out of absolute URLs.
/**
 * Preg replace callback: Makes links out of absolute URLs.
 *
 * @see preg_replace_callback.
 */

 * preg_replace callback to make links out of e-mail addresses.
/**
 * Preg replace callback: Makes links out of e-mail addresses.
 *
 * @see preg_replace_callback.
 */

 * preg_replace callback to make links out of domain names starting with "www."
/**
 * Preg replace callback: Makes links out of domain names starting with "www."
 *
 * @see preg_replace_callback.
 */
jhodgdon’s picture

Hmmm.

* Filter tips callback: Provides filter tips for the HTML filter.

This could be made a few characters shorter as:

Filter tips callback: Provides help for the HTML filter.

That would at least give us a few extra characters to work with. I think we are stuck with the name "Filter tips callback", because "tips callback" is what it is called in hook_filter_info(). These should probably have @see to the relevant hook_filter_info() implementation too? Oh, you already have that. Good.

On the other set:

* Preg replace callback: Makes links out of domain names starting with "www."

In this case, it is a callback for preg_replace(). We have a standard for this type of thing:
http://drupal.org/node/1354#callbacks
So this should be:

* Callback for preg_replace(): whatever.
NROTC_Webmaster’s picture

Status: Needs work » Needs review
FileSize
32.44 KB
35.1 KB

jhodgdon,

Thanks for the quick reply. The change you mentioned got all of comments under the 80 character limit.

Hopefully this patch corrects all of the issues identified so far.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks for the new patch!

I took a look at the current patch, all the way through. A few things that should be fixed:

a) In filter.admin.inc, there are a few form constructors that are missing @param docs (only needed for the parameters other than $form, $form_state).

b) In filter.admin.inc, there are a couple of page callbacks that are missing @param/@return. These are required under current standards:
http://drupal.org/node/1354#menu-callback

c) In filter.api.php -- for hook docs, the verb tense is different, so these changes need to be put back to what they are:

@@ -11,7 +11,7 @@
  */
 
 /**
- * Define content filters.
+ * Defines content filters.

http://drupal.org/node/1354#hooks

d) In filter.module:

@@ -996,7 +1020,25 @@ function filter_access($format, $account = NULL) {
...
+ * @return
+ *   An array, keyed by text format name. For each format, the value is another
+ *   array keyed by filter name. The values of the inner arrays are arrays
+ *   of the form:
+ *   @code
+ *     array(
+ *       'tip' => 'Tip text',
+ *       'id'  => FILTER_ID
+ *     )
+ *   @endcode
  */
 function _filter_tips($format_id, $long = FALSE) {

Normally we don't use @code to show structure of returned arrays. I would rewrite this @return section to say:
An associative array of filtering tips, keyed by filter name. Each filtering tip is an associative array with elements:
- tip: Tip text.
- id: Filter ID.

e) filter.module

@@ -1345,7 +1390,9 @@ function _filter_html_tips($filter, $format, $long = FALSE) {
 }
 
 /**
- * Settings callback for URL filter.
+ * Filter URL Settings callback: Provides settings for the URL filter.

Settings should not be capitalized.

f) filter.module

@@ -1579,21 +1634,23 @@ function _filter_url_trim($text, $length = NULL) {
...
 /**
- * Convert line breaks into <p> and <br> in an intelligent fashion.
+ * Converts line breaks into <p> and <br> in an intelligent fashion.
  * Based on: http://photomatt.net/scripts/autop
  */
 function _filter_autop($text) {

Needs newline after the initial doc line.

g) filter.module

@@ -1678,7 +1737,9 @@ function _filter_html_escape($text) {
 }
 
 /**
- * Filter tips callback for HTML escaping filter.
+ * Filter HTML escape tips callback: Provides help for an HTML escaping filter.

an -> the

That's it! The rest of the patch looks great. Thanks!

NROTC_Webmaster’s picture

Status: Needs work » Needs review
FileSize
34.8 KB

I took care of most of your comments.
For b) there were two that I did not include the return values for. filter_admin_format_form() where it returns the form and filter_admin_format_page() where it returns drupal_get_form('filter_admin_format_form', $format); I grepped core and couldn't find a single place these were documented with the return values.

For g) I changed it but that puts it at 81 characters.

jhodgdon’s picture

g) Actually, these callbacks should uniformly be called: "Filter tips callback: Specific description here." Normally these one-liners with : have a standard part before the colon, and the specific part after.

jhodgdon’s picture

Status: Needs review » Needs work

Oh, and on (b) - If it's a page callback, it should have a return value stating either that it returns a renderable array or a form array or a string or whatever. If it's a form constructor, it should be documented with the format of a form constructor (no return value necessary). So this one:

/**
- * Menu callback; Display a text format form.
+ * Page callback: Displays the text format add/edit form.
+ *
+ * @param $format
+ *   An associative array containing:
+ *   - format: The new format to be used.
+ *   - name: The name of the new format.
+ *
+ * @see filter_menu()
  */
 function filter_admin_format_page($format = NULL) {

still needs a return value, unless it should say "Form constructor for the...".

NROTC_Webmaster’s picture

Sorry for the delay.

I tried looking for an example of how to give a return value for

return drupal_get_form('filter_admin_format_form', $format);

but I can't find an example in the documentation that has already been fixed documenting a return value for drupal_get_form. I don't want to sound like I'm disagreeing with you but I don't know what the proper return value is here unless you want me to say "Returns a renderable form array for the filter_admin_format_form()." which doesn't seem right to me.

jhodgdon’s picture

@return
A form array.

would be enough, given that the one-line description says which form array it is. That is consistent with other page callback return statements we have been putting in core.

NROTC_Webmaster’s picture

Status: Needs work » Needs review
FileSize
3.9 KB
34.81 KB

Here is the latest.

Status: Needs review » Needs work

The last submitted patch, doc-cleanup-filter-module-1347914-28.patch, failed testing.

NROTC_Webmaster’s picture

Status: Needs work » Needs review
FileSize
34.81 KB

Status: Needs review » Needs work

The last submitted patch, doc-cleanup-filter-module-1347914-30.patch, failed testing.

NROTC_Webmaster’s picture

Status: Needs work » Needs review
FileSize
34.5 KB

Apparently the filter.css file was deleted. Hopefully this patch will apply.

xjm’s picture

Status: Needs review » Needs work

Thanks @NROTC_Webmaster. I reviewed the full patch again and found just a few things:

  1. +++ b/core/modules/filter/filter.admin.incundefined
    @@ -304,7 +331,9 @@ function filter_admin_format_form_validate($form, &$form_state) {
    + * Form submission hanlder for filter_admin_format_form().
    

    Typo: "hanlder".

  2. +++ b/core/modules/filter/filter.moduleundefined
    @@ -164,29 +168,32 @@ function filter_format_load($format_id) {
    + *     - status: (optional) A boolean indicating whether the filter is
    

    "Boolean" should be capitalized ("A Boolean indicating..."). (It's derived from a name.)

  3. +++ b/core/modules/filter/filter.moduleundefined
    @@ -1032,14 +1069,14 @@ function _filter_tips($format_id, $long = FALSE) {
    + *   The partial (X)HTML snippet to load. Invalid mark-up will be corrected on
    + *   import.
    

    I think "markup" should be one word (no hyphen).

  4. +++ b/core/modules/filter/filter.moduleundefined
    @@ -1054,15 +1091,16 @@ function filter_dom_load($text) {
    + * The function serializes the body part of a DOMDocument back to an XHTML
    + * snippet.
    ...
    + * The resulting XHTML snippet will be properly formatted to be compatible with
    + * HTML user agents.
    

    I wonder if these two sentences could be merged into one paragraph?

  5. +++ b/core/modules/filter/filter.testundefined
    @@ -764,7 +779,7 @@ class FilterSecurityTestCase extends DrupalWebTestCase {
    +   * Tests whehter filtered content is emptied when a the module is disabled.
    

    Typo ("whehter").

I'll also apply the patch locally and see if anything was missed. An interdiff showing the changed lines from the previous patch would again be helpful.

xjm’s picture

I found a few other things to fix when I reviewed the module with the patch applied:

  • FilterAdminTestCase, FilterFormatAccessTestCase, FilterDefaultFormatTestCase, and FilterNoFormatTestCase are still missing docblocks.
  • _filter_html_settings(), _filter_html(), _filter_url(), Drupal.behaviors.filterGuidelines, and several callback hooks in filter.api.php have nonstandard docblocks yet.
NROTC_Webmaster’s picture

Status: Needs work » Needs review
FileSize
25.47 KB
41.22 KB

I think I took care of all of the issues except the callbacks. I have a hard time understanding how they should be documented.

For _filter_html_settings should it be

/**
 * Settings callback: Settings for the HTML filter.
 *
 * @param $filter
 *   The filter object containing the current settings for the given format,
 *   in $filter->settings.
 * @param $format
 *   The format object being configured.
 * @param $defaults
 *   The default settings for the filter, as defined in 'default settings' in
 *   hook_filter_info(). These should be combined with $filter->settings to
 *   define the form element defaults.
 *
 * @return
 *   An array of form elements defining settings for the filter. Array keys
 *   should match the array keys in $filter->settings and $defaults.
 *
 * @see hook_filter_FILTER_settings()
 */
function _filter_html_settings($form, &$form_state, $filter, $format, $defaults) {

Additionally, when the docblock has @code is the code allowed to go past the 80 char limit? It doesn't describe it in the documentation so I didn't change it.

jhodgdon’s picture

@code can go past 80 characters I guess, but it would probably be nicer to break it up.

Regarding that filter settings callback... we haven't really made standards for these, but since there is "hook" documentation for hook_filter_FILTER_settings(), I think we could do:

/**
 * Filter settings callback for the HTML content filter.
 *
 * See hook_filter_FILTER_settings() for documentation of parameters and return value.
 */

Notes:
- That second line might need to be wrapped to fit 80 characters
- I took the term "content filter" from hook_filter_info().

NROTC_Webmaster’s picture

Updated per #36.
Interdiff from patch in 32.

jhodgdon’s picture

Status: Needs review » Needs work

It's looking pretty good! A few problems:

a) filter.admin.inc

@@ -262,6 +284,9 @@ function filter_admin_format_form($form, &$form_state, $format) {
  *   An associative array containing:
  *   - element: A render element representing the form.
  *
+ * @return
+ *   A renderable array.
+ *
  * @ingroup themeable
  */
 function theme_filter_admin_format_filter_order($variables) {

That @return is not accurate. This is a theme function and it returns HTML. We don't need the @return there actually. See http://drupal.org/node/1354#themeable

b) As long as we're fixing that (filter.admin.inc again):

@@ -336,10 +365,16 @@ function filter_admin_format_form_submit($form, &$form_state) {
 }
 
 /**
- * Menu callback; confirm deletion of a format.
+ * Form constructor for text format deletion confirmation form.

Could use a "the" in there.

c) And in quite a few places in filter.admin.inc:

+ * @param $format
+ *   An associative array containing:
+ *   - format: The format to be used.
+ *   - name: The name of the format.

This is unclear. If it's an associative array, what are the keys? And what is a "format" -- an array or an object, with what components? Is the name a machine name or a translated human-readable name?

d)

+++ b/core/modules/filter/filter.api.php
@@ -11,7 +11,7 @@
  */
 
 /**
- * Define content filters.
+ * Defines content filters.

Should be Define here. See http://drupal.org/node/1354#hooks -- and the same applies to the other hook definitions in this file.

e)

@@ -134,7 +135,7 @@ function hook_filter_info_alter(&$info) {
  * module will take care of saving the settings in the database.
  *
  * If the filter's behavior depends on an extensive list and/or external data
- * (e.g. a list of smileys, a list of glossary terms), then the filter module
+ * (e.g. a list of smileys, a list of glossary terms), then the Filter module
  * can choose to provide a separate, global configuration page rather than
  * per-text-format settings. In that case, the settings callback function
  * should provide a link to the separate settings page.

Actually, "filter" should stay lower-case here, as I think it's referring to a module that provides a filter to Drupal, not to the core Filter module.

f) filter.module

@@ -164,29 +168,32 @@ function filter_format_load($format_id) {
 }
 
 /**
- * Save a text format object to the database.
+ * Saves a text format object to the database.
  *
  * @param $format
  *   A format object using the properties:

How about "having" the properties in the last line here? There's another "using the properties" below in the same function header doc too. It doesn't make sense to me, because we're documenting what parameters to pass into the function here. Here's the other spot:

  *     assigned to the text format, keyed by the name of each filter and using
  *     the properties:

g) filter.module

 /**
- * Display a text format form title.
+ * Title callback: Displays a text format form title.
+ *
+ * @see filter_menu()
  */
 function filter_admin_format_title($format) {

This is not a form title. It's a page title, or really just the name of a text format being used as a page title. But the function is also called directly from filter_format_fallback_title() apparently (though maybe that function is never used?) so maybe we shouldn't even be documenting this as a title callback?

NROTC_Webmaster’s picture

In regards to c) it is documented that way 8 times in 3 files.

I guess what I'm asking is what would you like it to say.
* An associative array containing:
* - format: A string used as the primary key to describe the new format.
* - name: A string to label the new format.

I could also give an example like
-format: filtered_html
-name: Filtered HTML

The format and name are simply varchar(255) with format being the primary key. They reference what filters are applied to what roles. (Unless I missed something)

NROTC_Webmaster’s picture

Status: Needs work » Needs review
FileSize
8.92 KB
38.72 KB

Actually, since I'm about to leave I'm going to just go ahead and make the change as described. Please let me know if you think it should be something else.

jhodgdon’s picture

Status: Needs review » Needs work

OK, what's the array key? Ooooooh, I finally think I see what this actually is... If I have understood properly, then the docs should say something like this:

A one-element associative array whose key is the machine name for the format, and whose value is the human-readable name of the format.

Is that right? It sounds like that is what you are saying this contains? If it's really a multi-element array, then it should say something like:

An associative array of formats, keyed by machine name. Each value is an array with elements:
- format: The machine name of the format.
- name: The human-readable name assigned to the format.

But I haven't looked at the code to see what this array really is... please verify!

NROTC_Webmaster’s picture

Status: Needs work » Needs review
FileSize
10.13 KB
39.65 KB

Actually, I was wrong before. After going through this format is an object. Please review the latest patch to make sure it is correct. The first two instances are the same with the third being simplified because of the function.

interdiff from #37

xjm’s picture

@NROTC_Webmaster, can you create your interdiffs using git or with patchutils? The diff-of-diffs is hard to read. Thanks!

xjm’s picture

Status: Needs review » Needs work

The patch looks good. I didn't yet apply it locally or look up the individual functions for the new parameter and return documentation. I only found one issue in the patch itself, plus a question:

+++ b/core/modules/filter/filter.admin.incundefined
@@ -95,7 +99,25 @@ function theme_filter_admin_overview($variables) {
+ * @param $format
...
 function filter_admin_format_page($format = NULL) {

I think this parameter is optional, so we should document it as such and probably explain the fallback behavior.

+++ b/core/modules/filter/filter.moduleundefined
@@ -1249,7 +1290,9 @@ function _filter_html($text, $filter) {
- * Filter tips callback for HTML filter.
+ * Filter tips callback: Provides help for the HTML filter.

@@ -1347,7 +1390,9 @@ function _filter_html_tips($filter, $format, $long = FALSE) {
- * Settings callback for URL filter.
+ * Filter URL settings callback: Provides settings for the URL filter.
+ *
+ * @see filter_filter_info()

I'm now thinking we should maybe document these like the Render API callbacks... thoughts?

NROTC_Webmaster’s picture

I will definitely add the optional to the first item.
Regarding the second I don't have a preference either way. I personally just like to follow a standard that I can apply in every case to simplify my life and make things orderly. If we are going to go with the Render API callback form I would like to see it explained a little more clearly in the documentation. I personally find it a little vague. The main problem being that I'm not quite sure when to use it and when not to. Let me look through it and I will see if I can come up with something that makes sense to me and passes your standard as well.

jhodgdon’s picture

Regarding the question in #44, these filter callbacks are pretty much only used in Filter module, so I think it's OK to do them as they are in this patch... but what would they look like if they were "like Render API callbacks"?

NROTC_Webmaster’s picture

Actually I have a question for you.

What do you think is the best way to list this is as optional? I don't like what I have but I don't know of a better way.

 * @param object|null $format
 *   (optional) A format object having the properties, or can be null:
 *
NROTC_Webmaster’s picture

My assumption was that they would have

- Somewhere in the function, there should be a paragraph saying where the callback is assigned and what particular Render API callback it is being used for.

instead of just the @see

jhodgdon’s picture

* @param object|null $format
*   (optional) A format object having the properties, or can be null:
*

Um... for one thing, in text use NULL not null. But actually, I wouldn't say "or can be NULL" anyway. You already have object|null as the type, and (optional) there, and presumably it's $format = NULL in the function signature, so that clause doesn't add anything really?

I would just say:

(optional) An object representing a format, with the following properties:

NROTC_Webmaster’s picture

Status: Needs work » Needs review
FileSize
2.87 KB
41.64 KB

Here is an update and iterdiff from #42 with the exception of changing the callbacks.

jhodgdon’s picture

Status: Needs review » Needs work

Looks mostly good! I gave this patch a complete review (sorry for the delay!) and found the following problems:

a) in filter.admin.inc:

+ * @return
+ *   A renderable array.
+ *
  * @ingroup themeable
  */
 function theme_filter_admin_format_filter_order($variables) {

The return value here is a string, not a renderable array (this is a theme function!). But our standards say to omit the @return for theme functions anyway: http://drupal.org/node/1354#themeable

b) filter.admin.inc:

/**
- * Menu callback; confirm deletion of a format.
+ * Form constructor for the text format deletion confirmation form.
  *
...
  */
 function filter_admin_disable($form, &$form_state, $format) {

This is not accurate. This form is for *disabling*, not deleting, a format. The callback called _filter_disable_format_access() in filter.module has the same problem.

c) in filter.module - filter_process_format() docs:

+ *   using the text format id specified in #format or the user's default format

id -> ID

d) in filter.module:

/**
 * Returns HTML for a link to the more extensive filter tips.
 *
 * @ingroup themeable
 */
function theme_filter_tips_more_info() {

This type of function header should have been left alone, see http://drupal.org/node/1354#themeable -- several (maybe just two?) were changed like this:

- * Returns HTML for a link to the more extensive filter tips.
+ * Returns the HTML for a link to the more extensive filter tips.

e)

  * @param $escape
  *   (optional) Boolean whether to escape (TRUE) or unescape comments (FALSE).
  *   Defaults to neither. If TRUE, statically cached $comments are reset.
+ *
+ * Callback for preg_replace_callback() within _filter_url().
  */
 function _filter_url_escape_comments($match, $escape = NULL) {

The added line should come before the @param section.

f) filter.test:

+  /**
+   * Tests the text format adminstration functionality.
+   */
   function testFormatAdmin() {

typo: adminstration -> administration

batigolix’s picture

Assigned: Unassigned » batigolix

gonna give this a try

batigolix’s picture

The attached patch incorporates comments from #51.

Regarding comment #51 f: the file filter.test seems to be removed.

I added comments for functions in the core/modules/filter/lib/Drupal/filter/Tests that weren't documented yet.

jhodgdon’s picture

batigolix’s picture

thanks for the link. I *did* search through all files for the "adminstration" typo. The fixes are included in the patch at #54

batigolix’s picture

Status: Needs work » Needs review

changing the status

jhodgdon’s picture

jhodgdon’s picture

Status: Needs review » Needs work

This looks pretty good, thanks -- and sorry no one reviewed it until now... I did find a few problems:

a)

+ * @return
+ *   A renderable array.
+ *
  * @ingroup themeable
  */
 function theme_filter_admin_format_filter_order($variables) {

This is not accurate. The return value is an HTML string (that is what theme functions almost always return). Actually, you don't need a @return here anyway, see:
http://drupal.org/node/1354#themeable

b)

 /**
- * Process filter disable form submission.
+ * Form submission handler for filter_format_disable().
  */
 function filter_admin_disable_submit($form, &$form_state) {

This is the submission handler for filter_admin_disable() actually.

That's it -- the rest of the patch looks good! If we can get these two items changed, that would be great.

batigolix’s picture

allright, here's the new patch

batigolix’s picture

Status: Needs work » Needs review

set the status to review

jhodgdon’s picture

When you make a small change on a large patch, it's very helpful for reviewers if you can include an interdiff, so that they don't have to review the whole patch. Instructions:
http://drupal.org/node/1488712

jhodgdon’s picture

Status: Needs review » Needs work

This is looking really good! I went ahead and committed the patch, because it is 99.9% great. We need a quick follow-up to fix these three items:

a)

+++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterHooksTest.php
@@ -36,7 +36,7 @@ class FilterHooksTest extends WebTestBase {
   }
 
   /**
-   * Test that hooks run correctly on creating, editing, and deleting a text format.
+   * Tests that hooks run correctly on creating, editing, and deleting a text format.
    */
   function testFilterHooks() {

This line is longer than 80 characters.

b)

+++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterNoFormatTest.php
...
+  /**
+   * Tests if text with no format is filtered the same as text in the fallback
+   * format.
+   */
   function testCheckMarkupNoFormat() {
 

This needs to have a one-line 80-character summary.

c) same as (a):

+++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterSecurityTest.php
@@ -51,7 +51,7 @@ class FilterSecurityTest extends WebTestBase {
   }
 
   /**
-   * Test that filtered content is emptied when an actively used filter module is disabled.
+   * Tests that filtered content is emptied when an actively used filter module is disabled.
    */
   function testDisableFilterModule() {
batigolix’s picture

Status: Needs work » Needs review
FileSize
2.59 KB

attached patch incorporates comments from #63

Cameron Tod’s picture

Status: Needs review » Needs work

Hi batgolix, thanks for your help on this. It seems like your patch changes different docs than those mentioned in #63, could you double check that you're changing the right comments?

a)

+++ b/core/modules/dblog/lib/Drupal/dblog/Tests/DBLogTest.phpundefined
@@ -10,6 +10,9 @@ namespace Drupal\dblog\Tests;
+/**
+ * Tests logging messages in database
+ */

Out of scope for this issue.

b)

+++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterNoFormatTest.phpundefined
@@ -22,8 +22,10 @@ class FilterNoFormatTest extends WebTestBase {
+   * Tests if text with no format is filtered the same way as text in the ¶

There's a small trailing whitespace issue here.

c)

+++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterSecurityTest.phpundefined
@@ -51,7 +51,10 @@ class FilterSecurityTest extends WebTestBase {
-   * Tests that filtered content is emptied when an actively used filter module is disabled.
+   * Tests an actively used disabled filter.
+   *
+   * Tests that filtered content is emptied when an actively used filter module
+   * is disabled.
    */
   function testDisableFilterModule() {
     // Create a new node.

This is a little unclear, how about something like

"Tests removal of filtered content when an active filter is disabled."

batigolix’s picture

oops, that was a left over from another issue I ve been working on this morning.

attached a new patch that addresses the comments from #65

batigolix’s picture

Status: Needs work » Needs review

set status

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs review » Patch (to be ported)

Thanks! Sorry again for the delay on committing these patches... this one is committed. I think it's time to port both this latest patch and the one in #60 (combined, that is) to 7.x.

Lars Toomre’s picture

Version: 7.x-dev » 8.x-dev
Status: Patch (to be ported) » Needs review
FileSize
23.08 KB

I spent time this evening reading through the entire Filter module and saw that there still a number of issues with the tests in this module. Attached is a locally untested patch that documents what I found.

The filter callback functions in the middle of the filter.module file deserve another look. The documentation for those are inconsistent still and it is unclear to me which ones, if at all, need @param and @return directives.

I also opened issue #1811638: [meta] Clean-up Test members - ensure property definition and use of camelCase naming convention to address a separate coding convention issue that I noticed in the tests.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks! This all looks good except:

a)

- *   (optional) Boolean whether to escape (TRUE) or unescape comments (FALSE).
- *   Defaults to neither. If TRUE, statically cached $comments are reset.
+ *   (optional) A Boolean indicating whether to escape (TRUE) or unescape
+ *   comments (FALSE). Defaults to NULL. If TRUE, statically cached $comments
+ *   are reset.
 */
 function _filter_url_escape_comments($match, $escape = NULL) {

Some information was lost here -- it should probably say "Defaults to NULL, indicating neither." on that 2nd line.

b)

+  /**
+   * A user with administrative permissions.
+   *
+   * @var object
+   */
+
+  /**
+   * A user with content create and edit own page permissions.
+   *
+   * @var object
+   */
+

These appear to be stray lines added to FilterAdminTest.php that don't document anything?

c)


 /**
- * Tests for text format and filter CRUD operations.
+ * Tests for text format and Filter CRUD operations.
  */
 class FilterCrudTest extends WebTestBase {
 

I don't think "filter" here is referring to the "Filter module", but just "filters", so the previous lower-case text was actually correct. Also applies to some of the other classes.

d) Changes like this:

class FilterNoFormatTest extends WebTestBase {
+
   public static function getInfo() {
}
...
+
 }

I'm not aware that we have a standard saying that there should be a blank line at the end of a class just before the closing } or at the top. In any case, it's not a documentation standard, and these types of changes do not belong in this patch.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
4.69 KB
18.5 KB

I had to re-roll the initial patch to get an interdiff, and I accidentally left some of the extra newline removals out of the re-roll. Therefore the interdiff isn't complete, but the new patch is free of newlines.

jhodgdon’s picture

Status: Needs review » Needs work

There's a small indentation mishap here in filter.module, which I probably missed in the last review, sorry:

* @param $comment_end
- *   String to use as a comment end marker to escape the CDATA declaration.
+ *  (optional) A string to use as a comment end marker to escape the CDATA
+ *  declaration. Defaults to an empty string.
  */
 function filter_dom_serialize_escape_cdata_element($dom_document, $dom_element, $comment_start = '//', $comment_end = '') {

Everything else looks good, thanks!

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
829 bytes
18.5 KB

Here we go.

jhodgdon’s picture

Assigned: batigolix » jhodgdon
Status: Needs review » Reviewed & tested by the community

Thanks! I'll get this committed soon.

jhodgdon’s picture

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

Committed this patch!

I think it's now time to go back to 7.x and port a combination of this patch in #73 and the one in #66 and the one in #60, as much as possible anyway. Thanks!

Albert Volkman’s picture

Status: Patch (to be ported) » Needs review
FileSize
38.16 KB

Backported.

jhodgdon’s picture

Assigned: Unassigned » jhodgdon
Status: Needs review » Reviewed & tested by the community

Excellent work, as usual! This is ready for commit (can't do it right now, running out the door...). Thanks! I see a few places where we could go back to D8 and fix things again, but nothing major, so I think the best thing is just to get this FINISHED. :)

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 7.x, with the exception of the JS files (I've been asked not to add file doc headers to JS files for Drupal 7, as we are not minifying JS in D7 and it adds to overhead).

jhodgdon’s picture

Assigned: jhodgdon » Unassigned

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