The naming of the parameter $format_id in check_markup() is quite confusing. Actually it takes the machine name of a filter format as a string.
While you could say that this machine name is indeed the ID of the filter format, a numerical ID is what would be expected. This is especially the case after in D6, the same function took a numerical ID, even though there, the parameter was simply called $format.
Confused? Mee too.

Propose to rename the parameter back to $format and to rewrite the completely uncomprehesible API documentation. How about this:

$format The machine name of the filter format to be used to filter the text. If no format is assigned, the fallback format will be used, see filter_fallback_format().

CommentFileSizeAuthor
#22 core-1041486-fix_misleading_parameter-22.patch778 bytescs_shadow
#20 core-1041486-fix_misleading_parameter-20.patch1.33 KBcs_shadow
#18 core-1041486-fix_misleading_parameter-18.patch782 bytescs_shadow
#12 1041486-12.patch804 byteslokapujya
#10 check_markup_docs-1041486-10.patch787 byteseporama
#5 check_markup_docs-1041486-5.patch793 bytesPancho
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Pancho’s picture

Pancho
Location UTC+2 🇪🇺 EU
CreditAttribution: Pancho commented
Title: Documentation problem with check_markup » Confusing naming of $format_id in check_markup()
Component: documentation » filter.module
Priority: Normal » Minor
franz’s picture

franz
any
Location Montréal
CreditAttribution: franz commented
Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7

A patch would be nice to this.

joestewart’s picture

joestewart CreditAttribution: joestewart commented

Just adding a little context. This was changed in #582378: Filter System clean-up

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Active » Closed (won't fix)

Too minor, or someone would have patched it already.

Pancho’s picture

Pancho
Location UTC+2 🇪🇺 EU
CreditAttribution: Pancho commented
Title: Confusing naming of $format_id in check_markup() » Fix misleading parameter documentation in check_markup()
Status: Closed (won't fix) » Needs review
FileSize
check_markup_docs-1041486-5.patch793 bytes

The parameter's name is in line with the rest of the API functions, so that should stay now.
However, documentation of the parameter remains misleading:

- *   (optional) The format ID of the text to be filtered. If no format is
- *   assigned, the fallback format will be used. Defaults to NULL.
+ *   (optional) The ID of the text format to be used for filtering. If not 
+ *   given, fallback format will be used, @see filter_fallback_format().

It's not "the format ID of the text" but the "ID of the text format".
Also, while it technically defaults to NULL, in this case filter_fallback_format() is used, so that should be documented.

Agree this is minor, but an quick-and-simple API doc improvement.

franz’s picture

franz
any
Location Montréal
CreditAttribution: franz commented
Status: Needs review » Reviewed & tested by the community

Looks good.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Component: filter.module » documentation
Assigned: Unassigned » jhodgdon

Since this is a docs thing, sending onto jhodgdon. :)

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Reviewed & tested by the community » Needs work

You cannot use @see in the middle of a documentation line. Just use the word "See".

+ *   (optional) The ID of the text format to be used for filtering. If not
+ *   given, fallback format will be used, @see filter_fallback_format().

Also that comma before @see needs to be either a period or a semi-colon (as it is, it's a comma splice of two complete sentences). And the grammar of the second sentence needs to be cleaned up -- needs "the" or "a" or something in there before "fallback".

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Assigned: jhodgdon » Unassigned
Issue summary: View changes

This should not still be assigned to me.

eporama’s picture

eporama CreditAttribution: eporama commented
Assigned: Unassigned » eporama
Status: Needs work » Needs review
FileSize
check_markup_docs-1041486-10.patch787 bytes

Here is a quick patch for the doc. Changed to "Defaults to the fallback format" as the structure of "Defaults to" seems to be more common.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work

Thanks! But this doesn't really fix the original issue that was reported here, because it doesn't make clear it's the machine name of a format, not an integer ID.

lokapujya’s picture

lokapujya
Primary language English
Location Boston
CreditAttribution: lokapujya commented
Assigned: eporama » lokapujya
Status: Needs work » Needs review
FileSize
1041486-12.patch804 bytes

Updated the comment.

<?php
 *   (optional) The machine name of the filter format to be used to filter the
 *   text. Defaults to the fallback format. See filter_fallback_format().
?>
srikanthk16’s picture

srikanthk16 CreditAttribution: srikanthk16 commented
Status: Needs review » Fixed

**The patch is secure , since no core modifications are performed
**Theme functions/files are not altered, just documentation is changed
**no duplicate code
**Code is not altered
**Previous tests are sufficient
**provided patches are satisfying the alterations in discussion
so the issue is closed

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Fixed » Needs review
jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Reviewed & tested by the community

Thanks for the patch! It looks fine to me.

Regarding #13 - please read the note about "issue status" that is linked to in the "issue metadata" section -- https://drupal.org/node/156119 -- issues should not be marked "Fixed" until the patch has been committed. When you review a patch, you can change the status to "needs work" or "reviewed and tested".

Also, just because a patch doesn't change code and "just" changes documentation, doesn't mean it doesn't need a review. What we'd like if you are reviewing a documentation patch is to check whether the new documentation is accurate and solves the problem of the issue. Thanks!

So, I've reviewed the patch and it looks like good documentation and solves the ambiguity that was reported here. I'll get it committed shortly, unless one of the other committers beats me to it.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed and pushed to 8.x. Thanks!

Moving down to 7.x for backport.

  • Commit b6ffef2 on 8.x by webchick: 
    Issue #1041486 by lokapujya, eporama, Pancho: Fix misleading parameter...
cs_shadow’s picture

cs_shadow CreditAttribution: cs_shadow commented
Status: Patch (to be ported) » Needs review
FileSize
core-1041486-fix_misleading_parameter-18.patch782 bytes

Attaching patch in #12 backported to 7.x.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work

There is an extra blank line in the D7 patch. Please remove. There should not be blank lines between @param sections (there should be a blank line between the end of the @param section and the @return section though).

cs_shadow’s picture

cs_shadow CreditAttribution: cs_shadow commented
Status: Needs work » Needs review
FileSize
core-1041486-fix_misleading_parameter-20.patch1.33 KB

Removed the extra blank line.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work

Uh oh, that patch has a line removed in entity.inc?

cs_shadow’s picture

cs_shadow CreditAttribution: cs_shadow commented
Status: Needs work » Needs review
FileSize
core-1041486-fix_misleading_parameter-22.patch778 bytes

Sorry. That's what happen when you forget to do a 'git checkout' before making a patch.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Reviewed & tested by the community

Thanks, looks good!

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Reviewed & tested by the community » Fixed

Thanks! Committed to 7.x.

  • Commit 3fbabe4 on 7.x by jhodgdon: 
    Issue #1041486 by cs_shadow, lokapujya, eporama, Pancho: Fix docs for...

Status: Fixed » Closed (fixed)

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