Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#44051 closed defect (bug) (invalid)

Improve `WP_Privacy_Policy_Content::get_default_content()` readability

Reported by: desrosj's profile desrosj Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.9.6
Component: Privacy Keywords:
Focuses: Cc:

Description

The coding standards state "readability is more important than cleverness or brevity."

WP_Privacy_Policy_Content::get_default_content() has lots of $descr && $content .= statements.

This statement adds the string that follows .= to $content if $descr is passed to the function as true.

Replacing these with if ( $descr ) {} is much less clever, but more readable.

Attachments (3)

44051.diff (21.8 KB) - added by desrosj 6 years ago.
44051.2.diff (22.4 KB) - added by ovann86 6 years ago.
Change argument name from 'descr' to 'include_descriptions' to make it's role easier to understand.
44051.3.diff (13.7 KB) - added by ovann86 6 years ago.
Patch with only argument name changed from 'descr' to 'include_descriptions' to make it's role easier to understand.

Download all attachments as: .zip

Change History (24)

#1 @desrosj
6 years ago

  • Keywords has-patch gdpr added; has-patchg dpr removed

Related: #44048, #44050.

@desrosj
6 years ago

#2 @desrosj
6 years ago

  • Milestone changed from Awaiting Review to 4.9.7

@ovann86
6 years ago

Change argument name from 'descr' to 'include_descriptions' to make it's role easier to understand.

#3 @ovann86
6 years ago

I would like to suggest going one step further by changing the variable name from 'descr' to 'include_descriptions' to make its role easier to understand.

Whilst I'm not seeing this in the WordPress PHP coding standards, using full words (instead of abbreviations) that properly describe the variable is seen more often than not in the rest of core.

note: my patch also fixes a typo in the variable's description - from undet to under

Last edited 6 years ago by ovann86 (previous) (diff)

#4 @azaozz
6 years ago

This follows the pattern from script-loader.php:

did_action( 'init' ) && $scripts->localize()...

Don't think that is really "clever" code, pretty standard pattern for many repeated if ( true ) statements.

Replacing these with if ( $descr ) {} is much less clever, but more readable.

I'm not so sure this makes it more readable. The part that needs to be readable is the text strings rather than the conditionals. Multiple if were the first thing I tried but repeating if ( $descr ) { 20 times felt wrong. Also it makes the strings less readable because of the indentation.

Nevertheless I'm still not happy with the readability there. Maybe we can try something like:

$descr && $content .= '<div class="wp-suggested-text">';
          $content .= '<h2>' . __( 'Who we are' ) . '</h2>';
$descr && $content .= '<p class="privacy-policy-tutorial">' . __( 'In this section you should note your site URL, as well as the name of the company, organization, or individual behind it, and some accurate contact information.' ) . '</p>';
$descr && $content .= '<p class="privacy-policy-tutorial">' . __( 'The amount of information you may be required to show will vary depending on your local or national business regulations. You may, for example, be required to display a physical address, a registered address, or your company registration number.' ) . '</p>';
          $content .= '<p>' . $suggested_text . sprintf( __( 'Our website address is: %s.' ), get_bloginfo( 'url', 'display' ) ) . '</p>';

          $content .= '<h2>' . __( 'What personal data we collect and why we collect it' ) . '</h2>';

#5 @desrosj
6 years ago

  • Component changed from General to Privacy

Moving to the new Privacy component.

#6 @desrosj
6 years ago

  • Version set to 4.9.6

Marking privacy bugs as introduced in 4.9.6.

#7 @desrosj
6 years ago

  • Milestone changed from 4.9.7 to 4.9.8

Moving all tickets in 4.9.7 to 4.9.8.

#8 @desrosj
6 years ago

  • Keywords needs-refresh added; gdpr removed

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


6 years ago

#10 @SergeyBiryukov
6 years ago

The readability seems fine to me as is.

I agree with @ovann86 though that $descr should be renamed to $include_descriptions for clarity.

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


6 years ago

#12 @desrosj
6 years ago

  • Milestone changed from 4.9.8 to 4.9.9

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


6 years ago

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


6 years ago

#15 @desrosj
6 years ago

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

This needs a new patch with @ovann86's variable change suggestion only.

#16 @desrosj
6 years ago

  • Focuses coding-standards removed

@ovann86
6 years ago

Patch with only argument name changed from 'descr' to 'include_descriptions' to make it's role easier to understand.

#17 @ovann86
6 years ago

  • Keywords has-patch added; needs-patch removed

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


6 years ago

#19 @pento
6 years ago

  • Milestone changed from 4.9.9 to Future Release

This ticket was mentioned in Slack in #core-privacy by webdevlaw. View the logs.


6 years ago

#21 @garrett-eclipse
6 years ago

  • Keywords has-patch removed
  • Milestone Future Release deleted
  • Resolution set to invalid
  • Status changed from new to closed

Closing this as it no longer applies. In 5.0 the $blocks parameter was introduced and this function rewritten. The $descr param is already updated to $description. If you feel it should be $include_description to be more clear feel free to re-open with a refreshed patch. Thank you

Note: See TracTickets for help on using tickets.