WordPress.org

Make WordPress Core

Opened 15 months ago

Closed 15 months ago

Last modified 14 months 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 15 months ago.
Change exporter and eraser arrays to associative arrays
43931.2.diff (7.5 KB) - added by allendav 15 months ago.
Filters changed to also include the key(slug) of the eraser/exporter
43931.3.diff (8.2 KB) - added by desrosj 15 months ago.
43931.4.diff (8.2 KB) - added by allendav 15 months 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.


15 months ago

@allendav
15 months ago

Change exporter and eraser arrays to associative arrays

#2 @allendav
15 months ago

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

#3 @allendav
15 months 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
15 months ago

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

#5 @johnjamesjacoby
15 months ago

Patch looks like a nice improvement. +1

#6 follow-up: @ocean90
15 months 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
15 months 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
15 months ago

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

@allendav
15 months ago

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

@desrosj
15 months ago

#9 @desrosj
15 months 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
15 months ago

  • Keywords needs-refresh removed

#11 @desrosj
15 months ago

  • Keywords needs-testing added

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


15 months ago

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


15 months ago

@allendav
15 months ago

Fix sprintf arguments (two places)

#14 @iandunn
15 months 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
15 months 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
15 months 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
14 months ago

  • Component changed from General to Privacy

Moving to the new Privacy component.

Note: See TracTickets for help on using tickets.