WordPress.org

Make WordPress Core

Opened 4 months ago

Closed 7 days ago

Last modified 6 days ago

#39671 closed defect (bug) (fixed)

Customize: Further improve inline documentation

Reported by: westonruter Owned by: 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 3 months ago.
Improves inline documentation for add_partial
39671-1.patch (26.0 KB) - added by 4nickpick 3 months ago.
Adds and expands inline docs for partials, settings, sections, controls and panels.
39671-2.patch (26.2 KB) - added by 4nickpick 3 months ago.
tweak docs on Partial's sanitize_js_callback argument
39671.2.patch (10.1 KB) - added by sagarprajapati 2 months ago.
39671.3.patch (9.9 KB) - added by ketuchetan 5 weeks ago.
39671.4.patch (10.0 KB) - added by ketuchetan 5 weeks ago.
39671.5.patch (14.6 KB) - added by ketuchetan 4 weeks ago.
39671.6.patch (14.6 KB) - added by BharatKambariya 4 weeks ago.
39671.7.patch (34.4 KB) - added by mrahmadawais 2 weeks ago.
Enhancement: Improve inline documentation and fix several @access tags for Customizer
39671.8.diff (29.0 KB) - added by westonruter 11 days 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 8 days ago.
Restore require docs; remove method access tags; correct additional since tags

Download all attachments as: .zip

Change History (48)

#1 @mrahmadawais
4 months 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
4 months ago

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

#3 @mrahmadawais
4 months ago

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

@4nickpick
3 months ago

Improves inline documentation for add_partial

#4 @4nickpick
3 months 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 3 months ago by 4nickpick (previous) (diff)

#5 @grapplerulrich
3 months 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
3 months 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
3 months ago

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

#7 @4nickpick
3 months 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
3 months 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
3 months ago

tweak docs on Partial's sanitize_js_callback argument

#9 @4nickpick
3 months 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 3 months ago by 4nickpick (previous) (diff)

#10 @grapplerulrich
3 months 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
3 months ago

  • Milestone changed from 4.8 to 4.7.4

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


2 months ago

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


2 months ago

#14 @swissspidy
2 months 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
2 months 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.


8 weeks ago

#17 @swissspidy
6 weeks 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
5 weeks ago

@ketuchetan
5 weeks ago

#18 @ketuchetan
5 weeks 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
5 weeks 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
4 weeks ago

#20 @ketuchetan
4 weeks ago

Hi @ericdaams

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

Thanks,

#21 @westonruter
4 weeks 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
4 weeks ago

  • Keywords needs-refresh removed

Hi @westonruter

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

Thanks

#23 @westonruter
3 weeks ago

  • Milestone changed from 4.7.5 to 4.8

@mrahmadawais
2 weeks ago

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

#24 @mrahmadawais
2 weeks 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
11 days 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
11 days ago

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

#26 in reply to: ↑ 25 @mrahmadawais
10 days 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
8 days 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
8 days 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
8 days 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.


8 days ago

#30 in reply to: ↑ 28 ; follow-up: @mrahmadawais
8 days 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 8 days ago by mrahmadawais (previous) (diff)

#31 in reply to: ↑ 30 @westonruter
8 days 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 days 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 days 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 days 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 days 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
6 days 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
6 days 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

Note: See TracTickets for help on using tickets.