Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 6 years ago

#39671 closed defect (bug) (fixed)

Customize: Further improve inline documentation

Reported by: westonruter's profile westonruter Owned by: westonruter's profile westonruter
Milestone: 4.8 Priority: normal
Severity: normal Version:
Component: Customize Keywords: has-patch commit
Focuses: docs Cc:

Description

We need to do another pass on the customizer code to ensure that inline documentation is added to both PHP and JS. This is building on #27534, #27065, #21303 among others.

For example, per @grapplerulrich:

I would like to see parts of the documentation improved. For example: In WP_Customize_Selective_Refresh::add_partial( $id, $args ) there is no inline documentation what values can be provided for teh args. Only by reading the code I am able to work out what the variables are but it is not so easy as the args are all of the public variables in the class.

https://make.wordpress.org/design/2017/01/13/what-makes-a-great-customization-experience/#comment-24907

Attachments (11)

39671.patch (5.1 KB) - added by 4nickpick 7 years ago.
Improves inline documentation for add_partial
39671-1.patch (26.0 KB) - added by 4nickpick 7 years ago.
Adds and expands inline docs for partials, settings, sections, controls and panels.
39671-2.patch (26.2 KB) - added by 4nickpick 7 years ago.
tweak docs on Partial's sanitize_js_callback argument
39671.2.patch (10.1 KB) - added by sagarprajapati 7 years ago.
39671.3.patch (9.9 KB) - added by ketuchetan 7 years ago.
39671.4.patch (10.0 KB) - added by ketuchetan 7 years ago.
39671.5.patch (14.6 KB) - added by ketuchetan 7 years ago.
39671.6.patch (14.6 KB) - added by BharatKambariya 7 years ago.
39671.7.patch (34.4 KB) - added by mrahmadawais 7 years ago.
Enhancement: Improve inline documentation and fix several @access tags for Customizer
39671.8.diff (29.0 KB) - added by westonruter 7 years ago.
Fix a couple incorrect @since tags; revert changes to kses.php; improve a couple descriptions
39671.9.diff (24.3 KB) - added by westonruter 7 years ago.
Restore require docs; remove method access tags; correct additional since tags

Download all attachments as: .zip

Change History (49)

#1 @mrahmadawais
7 years ago

May I ask, what is the scope of this ticket. The amount of documentation needed is subjective. More info will help me manage my time if I adopt this one? @westonruter

#2 @westonruter
7 years ago

@mrahmadawais scope is phpdoc and jsdoc which are absent or lacking.

#3 @mrahmadawais
7 years ago

@westonruter I'll try and jump in this next week.

@4nickpick
7 years ago

Improves inline documentation for add_partial

#4 @4nickpick
7 years ago

  • Keywords dev-feedback added

I took a stab at adding documentation to the method specifically complained about. Would appreciate some feedback to make sure I'm in the right direction.

Last edited 7 years ago by 4nickpick (previous) (diff)

#5 @grapplerulrich
7 years ago

  • Focuses docs added

@4nickpick This looks good. I see you copied the documentation of the properties in class-wp-customize-partial.php :)

For $settings I would mention what happens if undefined e.g. "IDs for settings tied to the partial. If undefined, $id will be used.". I am not sure if we can somehow explain how the array should be structured. From my understanding they a list of IDs.

We should also add that the default for $type is default, $container_inclusive is false and $fallback_refresh is true.

#6 @grapplerulrich
7 years ago

These methods also also in need to improved documentation.

  • WP_Customize_Manager::add_panel()
  • WP_Customize_Manager::add_section()
  • WP_Customize_Manager::add_setting()
  • WP_Customize_Manager::add_control()

@4nickpick
7 years ago

Adds and expands inline docs for partials, settings, sections, controls and panels.

#7 @4nickpick
7 years ago

  • Keywords dev-feedback removed

In this case, the arrays inside the $args parameters are relatively straightforward. I expanded on them a little bit in 39671-1.patch.

The scope of this ticket also includes jsdocs. I'll be digging into those next.

#8 @grapplerulrich
7 years ago

This looks really good. It is going to make live so much easier.

The documentation in core for $sanitize_js_callback has not been fully clear. Instead of "Callback to filter a Customize setting value for use in Javascript" perhaps "Callback to convert a Customize PHP setting value to a value that is JSON serializable."

@4nickpick
7 years ago

tweak docs on Partial's sanitize_js_callback argument

#9 @4nickpick
7 years ago

Still digging into the js end of Customizer. If there's any particularly bad file/method/section, or somewhere you think would be a good starting point, that would be helpful.

I'm specifically looking into these files:
customize-base.js
customize-loader.js
customize-models.js
customize-preview.js
customize-preview-nav-menus.js
customize-preview-widgets.js
customize-selective-refresh.js
customize-views.js

A lot of methods missing docs completely, I'll be starting creating stubs for those.

Last edited 7 years ago by 4nickpick (previous) (diff)

#10 @grapplerulrich
7 years ago

  • Keywords has-patch added; needs-patch removed

I think inline documentation for these features would be good.

  • Selective Refresh JavaScript Events
  • Notifications
  • Client-side Validation

https://developer.wordpress.org/themes/customize-api/tools-for-improved-user-experience/
https://developer.wordpress.org/themes/customize-api/the-customizer-javascript-api/

#11 @DrewAPicture
7 years ago

  • Milestone changed from 4.8 to 4.7.4

This ticket was mentioned in Slack in #core by swissspidy. View the logs.


7 years ago

This ticket was mentioned in Slack in #core by swissspidy. View the logs.


7 years ago

#14 @swissspidy
7 years ago

  • Keywords needs-refresh added

A quick note on 39671-2.patch

For reference: https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/php/

The other areas mentioned above would need (separate) patches as well.

#15 @sagarprajapati
7 years ago

Hi @swissspidy

I have updated patch with the above-listed suggestions. Please check it from your side and let me know your suggestions.

Thanks

This ticket was mentioned in Slack in #core by swissspidy. View the logs.


7 years ago

#17 @swissspidy
7 years ago

  • Milestone changed from 4.7.4 to 4.7.5

A few observations:

  • Indentation and alignment is not always correct.
  • @since annotations are missing

Moving to 4.7.5 as we're not in a hurry to move this into the next release.

@ketuchetan
7 years ago

@ketuchetan
7 years ago

#18 @ketuchetan
7 years ago

Hello @swissspidy

I have added my patch with the proper indentation and added missing tag @since tag on the doc block

Thanks,

#19 @ericdaams
7 years ago

@sagarprajapati @ketuchetan Your patch is missing the changes to wp-includes/customize/class-wp-customize-selective-refresh.php that were in @4nickpick's patch. Was that intentional?

@ketuchetan
7 years ago

#20 @ketuchetan
7 years ago

Hi @ericdaams

I have included the wp-includes/customize/class-wp-customize-selective-refresh.php missing changes.

Thanks,

#21 @westonruter
7 years ago

The @since tags aren't correct. They need to reflect the version in which the aspect is added, not the version in which the @since tag is added. So much of them will be 3.4.0 when the Customizer was first introduced.

#22 @BharatKambariya
7 years ago

  • Keywords needs-refresh removed

Hi @westonruter

I have updated patch with @since changes. Please check it.

Thanks

#23 @westonruter
7 years ago

  • Milestone changed from 4.7.5 to 4.8

@mrahmadawais
7 years ago

Enhancement: Improve inline documentation and fix several @access tags for Customizer

#24 @mrahmadawais
7 years ago

@westonruter I spent some time today on this ticket today and have added my patch that addresses the following:

  • Add Inline Docs where missing
  • Fix wrong @access tags (found private access tags for public functions)
  • Consistent @access tags throughout these files
  • Consistent order of DocBlock tags throughout the files as per WP Docs Standards

Kindly, have a look at it when possible :)

#25 follow-up: @westonruter
7 years ago

There are changes to kses.php which seem to have snuck into the patch. Those need to be removed.

Looking through some of the @since tags, I can see that they're not all accurate. For example \WP_Customize_Control::$input_attrs was added in 4.0.0 not 3.4.0 (see #28477). Likewise, \WP_Customize_Setting::$validate_callback was added in 4.6.0 not 3.4.0 (see #34893). So some more QA is needed on the patch.

The changes to the phpdoc comments before each require_once in class-wp-customize-control.php shouldn't be necessary. In fact, I don't think phpdoc comments are even appropriate here.

@westonruter
7 years ago

Fix a couple incorrect @since tags; revert changes to kses.php; improve a couple descriptions

#26 in reply to: ↑ 25 @mrahmadawais
7 years ago

Replying to westonruter:

The changes to the phpdoc comments before each require_once in class-wp-customize-control.php shouldn't be necessary. In fact, I don't think phpdoc comments are even appropriate here.

I made those changes in line with the Doc Standards for Requires and Includes. I hope that clarifies it.

Is this ticket now ready to be committed? I think it's a good addition — in the next major version we can also take a pass on JS files.

#27 @westonruter
7 years ago

@mrahmadawais you're right about:

I made those changes in line with the Doc Standards for Requires and Includes. I hope that clarifies it.

The changes to the @access tags on class methods however doesn't seem right. In looking at Functions & Class Methods it says:

access: Only use for standalone functions if private. If the function is private it will be output with a message stating its intention for internal use.

So I think the presence of @access for all of the class methods actually should be removed, which also makes sense because it's entirely redundant with the member visibility keywords which is part of the PHP language itself.

#28 follow-up: @westonruter
7 years ago

It also seems the docs for use of @access on class member properties should also be updated to note that it shouldn't be used, as I think perhaps it could be a carryover from PHP 4 which used just plain var keywords and lacked any visibility at the PHP language level.

@westonruter
7 years ago

Restore require docs; remove method access tags; correct additional since tags

This ticket was mentioned in Slack in #docs by westonruter. View the logs.


7 years ago

#30 in reply to: ↑ 28 ; follow-up: @mrahmadawais
7 years ago

Replying to westonruter:

It also seems the docs for use of @access on class member properties should also be updated to note that it shouldn't be used, as I think perhaps it could be a carryover from PHP 4 which used just plain var keywords and lacked any visibility at the PHP language level.

That makes a lot of sense. Since I find them quite redundant as well. Though, being part of the guidelines, I did add them anyway.

So I think the presence of @access for all of the class methods actually should be removed, which also makes sense because it's entirely redundant with the member visibility keywords which is part of the PHP language itself.

I missed that. Thanks for letting me know.

Is there anything else left for this patch? I did take a cursory look at the last patch and there are certain functions for which access is not defined E.g. class-wp-customize-control.php at line # 484 the render_content() has protected access. But that's not defined in the DocBlock. Should it be?

Last edited 7 years ago by mrahmadawais (previous) (diff)

#31 in reply to: ↑ 30 @westonruter
7 years ago

Replying to mrahmadawais:

That makes a lot of sense. Since I find them quite redundant as well. Though, being part of the guidelines, I did add them anyway.

Actually, I interpret the guidelines to mean that @access should be used for functions not methods.

Is there anything else left for this patch? I did take a cursory look at the last patch and there are certain functions for which access is not defined E.g. class-wp-customize-control.php at line # 484 the render_content() has protected access. But that's not defined in the DocBlock. Should it be?

So I think essentially all of the @access tags actually should be removed from the docblocks for methods in these classes. I've tried to get confirmation on this from #docs: https://wordpress.slack.com/archives/C02RP4WU5/p1495151067056851

#32 follow-up: @westonruter
7 years ago

  • Keywords commit added; good-first-bug removed

Also just came across this:

Incidentally, the @access tag has no context with regard to a class itself. Visibility scope of public/protected/private only applies to class methods and class properties, not to a class itself. Further, it was added to phpDocumentor 1.x back in the PHP4 days, before such visibility scope was available in PHP at all (that was added in PHP5). Therefore, it's not actually useful anymore. Even in 1.x, if run using PHP5 against code written for PHP5, the code scope keywords would override whatever an @access tag said. I don't believe that phpDocumentor 2.x even bothered implementing the @access tag, and rightly so.

http://stackoverflow.com/a/20148459/93579

I'm going to commit 39671.9.diff.

#33 @westonruter
7 years ago

  • Owner set to westonruter
  • Resolution set to fixed
  • Status changed from new to closed

In 40804:

Docs: Improve phpdoc for WP_Customize_Manager, WP_Customize_Control, WP_Customize_Setting, and WP_Customize_Selective_Refresh.

Props 4nickpick, sagarprajapati, ketuchetan, BharatKambariya, mrahmadawais, westonruter.
Fixes #39671.

#34 in reply to: ↑ 32 @mrahmadawais
7 years ago

Replying to westonruter:

Also just came across this:

Incidentally, the @access tag has no context with regard to a class itself. Visibility scope of public/protected/private only applies to class methods and class properties, not to a class itself. Further, it was added to phpDocumentor 1.x back in the PHP4 days, before such visibility scope was available in PHP at all (that was added in PHP5). Therefore, it's not actually useful anymore. Even in 1.x, if run using PHP5 against code written for PHP5, the code scope keywords would override whatever an @access tag said. I don't believe that phpDocumentor 2.x even bothered implementing the @access tag, and rightly so.

http://stackoverflow.com/a/20148459/93579

I'm going to commit 39671.9.diff.

Great! I think we can address the @access tag along with JS files in WordPress 4.9 then.

#35 follow-up: @grapplerulrich
7 years ago

@westonruter Is there a reason why @access was removed in a few places, left in others and added too?

Do we need to create a new issue for the JS documentation?

#36 in reply to: ↑ 35 @westonruter
7 years ago

Replying to grapplerulrich:

@westonruter Is there a reason why @access was removed in a few places, left in others and added too?

Yes, see discussion at https://wordpress.slack.com/archives/C02RP4WU5/p1495224872995687

Do we need to create a new issue for the JS documentation?

There will always be need for more docs tickets :-)

#37 @grapplerulrich
7 years ago

Yes, see discussion at https://wordpress.slack.com/archives/C02RP4WU5/p1495224872995687

I saw the discussion. I would have in this case not made any changes to @access as in https://core.trac.wordpress.org/changeset/40804 it is not consistent.

There will always be need for more docs tickets :-)

Here is the new ticket #40831

#38 @westonruter
6 years ago

In 42039:

Customize: Fix phpdoc for params in WP_Customize_Manager::add_section().

Amends [40804].
See #39671.
Fixes #42372.

Note: See TracTickets for help on using tickets.