Make WordPress Core

Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#54191 closed enhancement (fixed)

There is no comment for the initialize function

Reported by: yagniksangani's profile yagniksangani Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.9 Priority: normal
Severity: normal Version:
Component: Toolbar Keywords: has-patch
Focuses: coding-standards Cc:

Description

Hello, there is a comment missing for the initialize function in file wp-includes/class-wp-admin-bar.php

Attachments (7)

wp_ss_ys.png (25.9 KB) - added by yagniksangani 3 years ago.
wp_ss2_ys.png (24.3 KB) - added by yagniksangani 3 years ago.
Also there is comment missing for the add_menus function on same file
54191.diff (513 bytes) - added by yagniksangani 3 years ago.
Patch
54191.2.diff (579 bytes) - added by yagniksangani 3 years ago.
New Diff File
54191.3.diff (712 bytes) - added by yagniksangani 2 years ago.
New Diff File
54191.php (626 bytes) - added by rehanali 2 years ago.
I have added the comments for the initializing function
54191.patch (626 bytes) - added by rehanali 2 years ago.
New file added

Download all attachments as: .zip

Change History (22)

@yagniksangani
3 years ago

Also there is comment missing for the add_menus function on same file

#1 follow-up: @audrasjb
3 years ago

Hello @yagniksangani, thanks for the ticket!

Indeed. Would you like to create a patch for this?

#2 @audrasjb
3 years ago

  • Component changed from General to Toolbar
  • Keywords needs-patch added

#3 in reply to: ↑ 1 @yagniksangani
3 years ago

  • Keywords has-patch added; needs-patch removed
  • Resolution set to invalid
  • Status changed from new to closed

Yes, I would like to create a patch for this.
Replying to audrasjb:

Hello @yagniksangani, thanks for the ticket!

Indeed. Would you like to create a patch for this?

#4 follow-up: @audrasjb
3 years ago

  • Keywords needs-patch added; has-patch removed
  • Resolution invalid deleted
  • Status changed from closed to reopened

#5 in reply to: ↑ 4 ; follow-up: @yagniksangani
3 years ago

Hello @audrasjb
Now, what do I need to do for this ticket, can you please guide me? As I'm new to core contribution.

#6 in reply to: ↑ 5 @hellofromTonya
3 years ago

Replying to yagniksangani:

Now, what do I need to do for this ticket, can you please guide me? As I'm new to core contribution.

Hello @yagniksangani, Here are a couple of resources to help you get started creating and submitting a patch:

For creating the patch, you're very close with the pngs you uploaded.

  • Checkout the master branch (the images show you are on the 5.8 branch)
  • Sync your copy of master to the upstream wordpress/wordpress-develop repo, i.e. by pulling the latest changes from upstream and then merging these changes into your local copy
  • Add the comment change
  • Then svn diff > 54191.diff to create the patch file
  • Then upload that patch file to this ticket

@yagniksangani
3 years ago

Patch

#7 @hellofromTonya
3 years ago

Thank you @yagniksangani for your patch! w00t!

I'd like to offer you some feedback to further improve the method DocBlocks.

The start of the DocBlock is a summary that describes "what" the function/method does. It should be clear, simple, and brief. It's purpose (the reason it exists) is to introduce the function/method in such a way that it tells folks what behavior it will do when calling/invoking it. Functions and methods do work. Start with a verb such as Sets up, Initializes, Adds, Removes, etc.

Here's the handbook page for more information.

I often find inspiration by exploring like named functions/methods within the codebase. In searching the codebase, there's only one other named initialize, but other initializers are called init. (Tip: Search function init.) A couple of common pattern are:

<?php
/**
 * Set up the hooks for the [thing].
 *
 * @since X.X.X
 */

and

<?php
/**
 * Initialize the [thing].
 *
 * @since X.X.X
 */

For this particular initialize() method, it's doing both: setting up the hooks and initializing the admin bar.

What @since?
This is the version number when the method was introduced.

How can you discover which version number?
There are a few techniques:

  • Look at the class' version number. Then go to the branch to determine if this method was first introduced with the class. Results: Yes it was found here.
  • Use Trac blame feature, though this method is not as straightforward for finding what version introduced the code. It's great for finding the changeset and history of changesets.

For the initialize() method, maybe something like:

/**
 * Initializes the admin bar.
 *
 * @since 3.1.0
 */

Want to give this a try for improving both methods?

#8 @audrasjb
3 years ago

  • Keywords has-patch needs-refresh added; needs-patch removed

Thanks for the patch!

Aside from Tonya’s comments, I'd add another thing to note: it's better to generate the diff file in the wordpress-develop folder because that's easier for other people to apply your patch on their side. On this particular patch, that's not an issue but it can make things harder to test for future ones :-)

(so in your case, the path of the file will be src/wp-includes/class-wp-admin-bar.php rather than wp-includes/class-wp-admin-bar.php)

@yagniksangani
3 years ago

New Diff File

#9 @sabernhardt
3 years ago

  • Keywords needs-refresh removed
  • Milestone changed from Awaiting Review to 5.9

The updated patch looks good.

We could also consider adding a DocBlock for the public render function (not the protected _render).

#10 @mukesh27
2 years ago

If we don't have doc block description for render then we can add @since version number.

@yagniksangani
2 years ago

New Diff File

@rehanali
2 years ago

I have added the comments for the initializing function

#11 @rehanali
2 years ago

  • Component changed from Toolbar to Administration
  • Focuses ui added
  • Resolution set to invalid
  • Status changed from reopened to closed
  • Type changed from enhancement to feature request

@rehanali
2 years ago

New file added

#12 @audrasjb
2 years ago

  • Component changed from Administration to Toolbar
  • Focuses ui removed
  • Type changed from feature request to enhancement

@rehanali please do not change the ticket properties when it's not needed

It's not a feature request, it's not an UI ticket, it's not invalid, and it's not an Administration ticket.

#13 @audrasjb
2 years ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

#14 @SergeyBiryukov
2 years ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from reopened to closed

In 51876:

Docs: Improve documentation for WP_Admin_Bar methods.

Add some missing descriptions and @since tags.

Follow-up to [15671], [19120], [19429], [19501], [19558], [25475], [25478], [25563], [32534], [35157], [40947], [45821], [48826].

Props yagniksangani, hellofromTonya, audrasjb, sabernhardt, mukesh27, rehanali, SergeyBiryukov.
Fixes #54191.

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


2 years ago

Note: See TracTickets for help on using tickets.