Problem/Motivation

xjm made a code review after #2825204-116: REST views: authentication is broken got committed, let's address this here.

Proposed resolution

Address the point from the review.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#5 2917527-5.patch3.47 KBwim leers
#5 interdiff.txt1.81 KBwim leers
#4 2917527-4.patch3.53 KBdawehner
#2 2917527-2.patch1.81 KBdawehner

Comments

dawehner created an issue. See original summary.

dawehner’s picture

dawehner
Primary language German
commented
Issue summary: View changes
StatusFileSize
new 2917527-2.patch1.81 KB

Here is a adressing some of the points. I improved to actually explain properly what $authentication_providers contains.

dawehner’s picture

dawehner
Primary language German
commented
dawehner’s picture

dawehner
Primary language German
commented
Status: Active » Needs review
StatusFileSize
new 2917527-4.patch3.53 KB

This comment is a little weird. I think we can fix it by removing the hyphen (or is it an emdash? :P) and comma. Also it's not usual to use "choosing" without an object. I can't really parse out what it means to suggest an improvement, unfortunately. Is this true?

$authentication_providers is an array of module names for modules that provide each authentication method,
keyed by the authentication method.
Use the method names,
not the module names,
in the $authenticationProviders property.
But it seems really confusing to have the parameter and the property with almost-the-same-name but totally different values. Can we instead change or deprecate one or the other?

Maybe it would be good to reference where the structure of authenticationProviders if it's documented elsewhere?

This patch now does the following bits:

a) It keeps the existing property around. You never know, maybe someone relied on its behaviour
b) It adds a new property with a better name
c) it clearly documents what is in there

I think overall, this is a really nice small improvement.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new interdiff.txt1.81 KB
new 2917527-5.patch3.47 KB

Looks good! I only have nits, fixed them for you.

  1. +++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
@@ -87,10 +87,20 @@ class RestExport extends PathPluginBase implements ResponseDisplayPluginInterfac
+   * @deprecated as of 8.3.x, will be removed in before Drupal 9.0.0, see

    8.4.x 

  2. +++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
@@ -87,10 +87,20 @@ class RestExport extends PathPluginBase implements ResponseDisplayPluginInterfac
+   *   ¶

    Trailing whitespace. (Fix not in interdiff, my git setup automatically fixes trailing whitespace.) 

  3. +++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
@@ -124,9 +134,13 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
+    // $authentication_providers as defined in \Drupal\Core\DependencyInjection\Compiler\AuthenticationProviderPass

    Probably this class name belongs on the next line.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Closed (duplicate)

Merging this back into the main issue. Thanks!