Make WordPress Core

Opened 3 years ago

Closed 22 months ago

#51439 closed defect (bug) (fixed)

Docs: Undocumented @global instances

Reported by: ravipatel's profile ravipatel Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.1 Priority: low
Severity: normal Version: 4.2
Component: General Keywords: has-patch
Focuses: docs, coding-standards Cc:

Description

@global wpdb not define on line no 230.

display_setup_form() on define but not call outside of the function.

Attachments (6)

51439-install.php.patch (720 bytes) - added by ravipatel 3 years ago.
codeing standard undefine varible.
51439-install.php.2.patch (759 bytes) - added by ravipatel 3 years ago.
Added with full path for undefined varibale
51439-ajax-actions.php.patch (783 bytes) - added by ravipatel 3 years ago.
Added file patch another file - undefined variable
51439.patch (5.3 KB) - added by ravipatel 3 years ago.
Combile all chnages + $post_id missing to define on comment file.
51439-edit-form-comment.php.patch (739 bytes) - added by ravipatel 3 years ago.
$comment & $action not defined edit-form-comment.php
51439.diff (4.4 KB) - added by SergeyBiryukov 2 years ago.

Download all attachments as: .zip

Change History (17)

@ravipatel
3 years ago

codeing standard undefine varible.

#1 @ravipatel
3 years ago

  • Version set to trunk

#2 @davidbaumwald
3 years ago

  • Keywords needs-refresh added
  • Priority changed from normal to low

@ravipatel Can you refresh these patches against the src? The patch does not include the file path and is more difficult to apply. For instance, install.php should be src/wp-admin/install.php.

Here's a Handbook page about creating the patch from src/ https://make.wordpress.org/core/handbook/tutorials/trac/submitting-a-patch/

Last edited 3 years ago by davidbaumwald (previous) (diff)

@ravipatel
3 years ago

Added with full path for undefined varibale

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


3 years ago

@ravipatel
3 years ago

Added file patch another file - undefined variable

@ravipatel
3 years ago

Combile all chnages + $post_id missing to define on comment file.

@ravipatel
3 years ago

$comment & $action not defined edit-form-comment.php

#4 @davidbaumwald
3 years ago

  • Focuses docs added
  • Keywords close added
  • Summary changed from Undefined Varible - Codeing Standard to Docs: Undocumented @global instances

@ravipatel Looking at these patches more in depth, it looks like most of these added global vars are not necessary, as they are already defined earlier in execution. For example, this patch is not necessary. edit-form-comment.php is an included template, and both $action and $comment are defined in the parent file src/wp-admin/comment.php. Same goes for the $typenow and $wpdb in src/wp-admin/edit.php.

I'm adding the close keyword because I think this ticket is not needed. I'll leave it open for a couple of days to see if anyone think s this does deserve to be a standalone ticket. Please ensure that these variables are indeed undefined at runtime, if they aren't I suggest adding to this ticket instead.

#5 @ravipatel
3 years ago

@davidbaumwald
OK, but applied some changes which is right for a declaration for variables as per PHP code doc format.

#6 @hellofromTonya
3 years ago

  • Version changed from trunk to 4.2

The changes in the install.php file are from Version 4.2 rather than 5.6 (trunk), ie per Ticket #30799. Changing the Version to 4.2.

@SergeyBiryukov
2 years ago

#7 @SergeyBiryukov
2 years ago

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

Hi there, thanks for the patches!

I have checked the suggested changes and I agree with comment:4 that they don't seem to be necessary. These globals are not declared or documented in these particular places because they are all declared and documented elsewhere.

For example, [45233] / #46602 removed the $wpdb declaration from wp-admin/install.php, because the variable is already available there, it was previously declared in wp-load.php. As noted in comment:2:ticket:46602:

Per the documentation standards, the @global tag is meant to list PHP globals used within functions or methods.

Looking at other files in wp-admin, it seems like we don't generally document $wpdb in code that itself is in the global namespace. The standards don't say anything about it either.

The same goes for other variables in the patch too, they may not be explicitly declared in these specific instances but they are defined. If a static analysis tool flags them as undefined, that would be a false positive.

On second thought, if the goal here is to improve developer experience, I guess there might be some benefit in declaring these globals explicitly. I think they should be declared closer to where they are used, so that the context is immediately clear, and there is no need to redefine them. See 51439.diff, moving to 6.0 for consideration.

As for adding the @var keyword in wp-admin/credits.php and wp-admin/includes/ajax-actions.php: WordPress generally documents class properties or constants like that, but not local variables, so I don't think this is needed.

As for declaring the $wp_version global in wp-admin/includes/class-core-upgrader.php: the current code specifically imports it as a local variable (we do that in a few other places as well), so I think there is no need to declare it as a global. Doing that might have unintended side effects, like changing the global value instead of the local one.

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


23 months ago

#9 @costdev
23 months ago

  • Milestone changed from 6.0 to 6.1

Per the discussion in the bug scrub, I'm moving this ticket to the 6.1 milestone for now. If a committer finds that this patch is ready for commit, feel free to pull it back into the 6.0 milestone for inclusion in the release.

#10 @SergeyBiryukov
22 months ago

  • Owner set to SergeyBiryukov
  • Status changed from new to accepted

#11 @SergeyBiryukov
22 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 53450:

Docs: Explicitly declare some globals for clarity.

This aims to improve developer experience by making it clear that these variables are defined elsewhere.

Props ravipatel, davidbaumwald, hellofromTonya, costdev, SergeyBiryukov.
Fixes #51439.

Note: See TracTickets for help on using tickets.