Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#43850 closed enhancement (fixed)

Add privacy policy URL template tags

Reported by: iandunn's profile iandunn Owned by: iandunn's profile iandunn
Milestone: 4.9.6 Priority: normal
Severity: normal Version: 5.1
Component: Privacy Keywords: gdpr fixed-major has-dev-note
Focuses: ui, administration Cc:

Description

( This is a split off of ticket:43620#comment:30, since this has grown enough to warrant its own ticket. )


#43620 introduced get_privacy_policy_url() for retrieving the URL of the Privacy Policy page. Themes and plugins are going to need template tags and a filter in order to work modify the URL and generate links to it.

The latest patch before splitting the tickets was 43620.policy_url.policy_link_4.diff.

Attachments (2)

43850.diff (7.8 KB) - added by birgire 6 years ago.
43850.2.diff (10.3 KB) - added by iandunn 6 years ago.
Merge tests, move get_privacy_policy_url() tests to new url folder

Download all attachments as: .zip

Change History (18)

#1 @iandunn
6 years ago

Replying to 43620:comment:43 birgire:

I would suggest adding numbered placeholders.

Hmmm, what would be the advantage of that, since the string isn't translated?

I've finished the unit-tests for both get_privacy_policy_url() and get_the_privacy_policy_link()

Doh, so did I.

Do you want to upload the patch here and I'll combine them?

but I'm scratching my head to what ticket I should add them.

I used tests/phpunit/tests/url.php, since that's where some other link-template.php functions are tested. I'm open to other ideas, though.

#2 @birgire
6 years ago

@iandunn Thanks for creating the ticket, it makes things easier to reference.

No problem, I just attached what I had done so far, just in case something might be of any use here.

Regarding the (multiple) numbered placeholders, when not used for translation, it's more of a preference. We see it e.g. in the default themes.

There were few tests for each function in 43850.diff, so I preferred creating a new file for each one. But it looks like I used the wrong link group reference but should have taken the url group reference ;-)

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

@birgire
6 years ago

@iandunn
6 years ago

Merge tests, move get_privacy_policy_url() tests to new url folder

#3 follow-up: @iandunn
6 years ago

43850.2.diff merges the tests from 43620.policy_url.policy_link_4.diff and 43850.diff. @birgire, can you take a look and let me know what you think?

Based on the the docs, it sounds like tests/link/getThePrivacyPolicyLink.php and tests/url/getThePrivacyUrl.php are the best places for these. test/url/ didn't exist previously, so I created it. There are a lot of tests in tests/url.php, but it seems like they're an artifact from before the guideline was to create separate files for each function being tested.

I still need to go over the patch with fresh eyes tomorrow, but I think it's in pretty good shape. @azaozz, do you mind giving the tests a quick glance, in case I missed anything? The functions in link-template.php haven't been changed, so they should still be good to go.

#4 follow-up: @birgire
6 years ago

@iandunn Thanks for the merge, that looks good

Here are few suggestions:

  • Change Tests_Link_GetPrivacyPolicyUrl() to Tests_Url_GetPrivacyPolicyUrl().
  • The tag @subpackage seems to be on it's way out in the future according to the Handbook here.
  • The cleanup must come before assertions, else the cleanup might not run on a failure. We should remove the filter callbacks before the assertion.


  • Add a parameter comment here:
    * @param WP_UnitTest_Factory $factory
    
  • Add member variable doc comments here:
protected static $privacy_policy_page_id;
protected static $privacy_policy_url;
protected static $before;
protected static $after;

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

Replying to iandunn:

All looks good here. Good job! :)

#6 in reply to: ↑ 4 @iandunn
6 years ago

Replying to birgire:

  • Change Tests_Link_GetPrivacyPolicyUrl() to Tests_Url_GetPrivacyPolicyUrl().

Good catch :)

  • The tag @subpackage seems to be on it's way out

Ah, I hadn't seen that, thanks for pointing it out. It sounds like we're keeping @subpackage for the mean time, though?

For the time being, and for the sake of consistency, WordPress Core will continue to use @subpackage tags – both when writing new DocBlocks, and editing old ones.

Only when the new – external – PSR-5 recommendations are finalized, will across-the-board changes be considered, such as deprecating certain tags.

  • The cleanup must come before assertions, else the cleanup might not run on a failure. We should remove the filter callbacks before the assertion.

I think hook callbacks are automatically removed after each test is ran, regardless of the assertions:

...all actions and filters are reset to their original state after each test as well, so it is not necessary to manually remove hooks that have been added in a test.

That works in my tests. So technically, we don't even need to call remove_filter(), since there aren't any assertions after it, but it still seems good to include it, just to be explicit and avoid future problems if more assertions are added.

  • Add a parameter comment here:
  • Add member variable doc comments here:

Done, thanks!

#7 @iandunn
6 years ago

In 43002:

Privacy: Add template tags for building link to privacy policy page.

This introduces the get_the_privacy_policy_link() and the_privacy_policy_link() functions, as well as the privacy_policy_url filter.

A new tests/url/ folder was added to better organize tests related to get_*_url() functions. Previously, those tests were placed in tests/url.php and tests/link/, but neither of those locations are optimal. Placing tests in tests/url.php violates the guideline of creating separate files/classes for each function under test, and using tests/link/ conflates two distinct -- albeit related -- groups of functions. Over time, URL-related tests can be migrated to the new folder.

Props birgire, xkon, azaozz, iandunn.
See #43850.

#8 @iandunn
6 years ago

  • Keywords needs-testing has-patch removed

r43002 is all that we need to do here, AFAIK, but I left the ticket open because a permanent committer will need to backport this to the 4.9 branch.

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


6 years ago

#10 @azaozz
6 years ago

  • Keywords commit fixed-major added

#11 @joemcgill
6 years ago

Looks like this needs to wait on the changes from #43620, specifically [42980], to be backported before this can be merged to the 4.9 branch.

This ticket was mentioned in Slack in #gdpr-compliance by desrosj. View the logs.


6 years ago

#13 @SergeyBiryukov
6 years ago

In 43109:

Privacy: Add template tags for building link to privacy policy page.

This introduces the get_the_privacy_policy_link() and the_privacy_policy_link() functions, as well as the privacy_policy_url filter.

A new tests/url/ folder was added to better organize tests related to get_*_url() functions. Previously, those tests were placed in tests/url.php and tests/link/, but neither of those locations are optimal. Placing tests in tests/url.php violates the guideline of creating separate files/classes for each function under test, and using tests/link/ conflates two distinct -- albeit related -- groups of functions. Over time, URL-related tests can be migrated to the new folder.

Props birgire, xkon, azaozz, iandunn.
Merges [43002] to the 4.9 branch.
See #43850.

#14 @SergeyBiryukov
6 years ago

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

#15 @desrosj
6 years ago

  • Component changed from General to Privacy

Moving to the new Privacy component.

Note: See TracTickets for help on using tickets.