WordPress.org

Make WordPress Core

Opened 3 weeks ago

Closed 3 weeks ago

Last modified 9 days ago

#43931 closed enhancement (fixed)

Use associative array instead of numeric for exporters and erasers

Reported by: desrosj Owned by: iandunn
Milestone: 4.9.6 Priority: high
Severity: normal Version:
Component: Privacy Keywords: gdpr fixed-major
Focuses: Cc:

Description

In https://core.trac.wordpress.org/ticket/43438#comment:34, it was suggested to use associative keys for the personal data exporters.

I think this is a good idea and should be done (and something that has to be done before the first iteration of these features is released).

Say a plugin adds their exporter, but they add it at the beginning of the list using array_unshift(). Or, a plugin just reorders the exporters. If a plugin hardcodes a change to the exporter at index 2, that is now inconsistent.

Using associative keys also allows things to be changed without having to iterate over every exporter in the list to find the one you need.

Attachments (4)

43931.diff (3.8 KB) - added by allendav 3 weeks ago.
Change exporter and eraser arrays to associative arrays
43931.2.diff (7.5 KB) - added by allendav 3 weeks ago.
Filters changed to also include the key(slug) of the eraser/exporter
43931.3.diff (8.2 KB) - added by desrosj 3 weeks ago.
43931.4.diff (8.2 KB) - added by allendav 3 weeks ago.
Fix sprintf arguments (two places)

Download all attachments as: .zip

Change History (21)

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


3 weeks ago

@allendav
3 weeks ago

Change exporter and eraser arrays to associative arrays

#2 @allendav
3 weeks ago

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

#3 @allendav
3 weeks ago

Although plugins can continue to just push onto the exporter or eraser arrays in the filters, it would be better (for consistency) if they now used a slug, e.g.:

<?php
function register_my_plugin_exporter( $exporters ) {
  $exporters['comment-location-plugin'] = array(
    'exporter_friendly_name' => __( 'Comment Location Plugin' ),
    'callback'               => 'my_plugin_exporter',
  );
  return $exporters;
}

#4 @desrosj
3 weeks ago

  • Keywords needs-patch added; has-patch needs-testing removed
  • Type changed from defect (bug) to enhancement

#5 @johnjamesjacoby
3 weeks ago

Patch looks like a nice improvement. +1

#6 follow-up: @ocean90
3 weeks ago

  • Priority changed from normal to high

The wp_privacy_personal_data_erasure_page filter should probably be changed to include the slug of the erasure instead of an arbitrary index number

Although plugins can continue to just push onto the exporter or eraser arrays in the filters, it would be better (for consistency) if they now used a slug,

Since this is a new API we can and should enforce that. This would simplify some of the odd error messages like "index cannot be less than one." or "index is out of range".

This should be solved before the first beta gets released.

#7 in reply to: ↑ 6 @allendav
3 weeks ago

Replying to ocean90:

The wp_privacy_personal_data_erasure_page filter should probably be changed to include the slug of the erasure instead of an arbitrary index number

Although plugins can continue to just push onto the exporter or eraser arrays in the filters, it would be better (for consistency) if they now used a slug,

Since this is a new API we can and should enforce that. This would simplify some of the odd error messages like "index cannot be less than one." or "index is out of range".

This should be solved before the first beta gets released.

Good idea. Working this now.

#8 @desrosj
3 weeks ago

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

@allendav
3 weeks ago

Filters changed to also include the key(slug) of the eraser/exporter

@desrosj
3 weeks ago

#9 @desrosj
3 weeks ago

43931.3.diff adds to 43931.2.diff by updating the error messages to use the text key instead of the numeric key.

#10 @desrosj
3 weeks ago

  • Keywords needs-refresh removed

#11 @desrosj
3 weeks ago

  • Keywords needs-testing added

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


3 weeks ago

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


3 weeks ago

@allendav
3 weeks ago

Fix sprintf arguments (two places)

#14 @iandunn
3 weeks ago

  • Owner set to iandunn
  • Resolution set to fixed
  • Status changed from new to closed

In 43154:

Privacy: Store plugin callbacks in associative array for flexibility.

The personal data export and erasure tools allow plugins to register their own callbacks, in order to add additional data to the export and erasure processes. Previously, these were registered without specifying a constant identifier in the array of callbacks. Using mutable integers makes it difficult for plugins to modify the callbacks of other plugins, though.

Using associative array keys instead provides a covenient and reliable way to identify and interact with another plugin's callbacks.

Props desrosj, allendav, ocean90.
Fixes #43931.

#15 @iandunn
3 weeks ago

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

Reopening for backport to 4.9.

#16 @azaozz
3 weeks ago

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

In 43157:

Privacy: Store plugin callbacks in associative array for flexibility.

The personal data export and erasure tools allow plugins to register their own callbacks, in order to add additional data to the export and erasure processes. Previously, these were registered without specifying a constant identifier in the array of callbacks. Using mutable integers makes it difficult for plugins to modify the callbacks of other plugins, though.

Using associative array keys instead provides a covenient and reliable way to identify and interact with another plugin's callbacks.

Props desrosj, allendav, ocean90.
Merges [43154] to the 4.9 branch.
Fixes #43931.

#17 @desrosj
9 days ago

  • Component changed from General to Privacy

Moving to the new Privacy component.

Note: See TracTickets for help on using tickets.