EmbedInsertCommand
  1. Nit: Implements … -> @inheritdoc
  2. Nit: array() -> []
EmbedButtonInterface
  1. […] of the embed type for which this button is enabled is in the docs 3 times, but doesn't make sense. […] of the associated embed type makes more sense and is shorter.
  2. Punctuation nits on getTypeSetting()
  3. It is very confusing to see both getIconFile() and getIconUrl(): the person reading this interface will wonder why you can't just get the file URL from the return value of getIconFile(). Turns out there's a good reason: because if no custom icon file is specified, the default icon file for the embed type is used. The docs should say this.
EmbedButton
  1. Nit: array() -> []
  2. Nit: t() -> $this->t() (needed use StringTranslationTrait; for that)
  3. Nit: no blank line between two methods.
EmbedButtonListBuilder
  1. Nit: Unused use statement.
  2. Nit: Provides a listing of EmbedButton is invalid English.
  3. Nit: array() -> []
  4. Nit: '#alt' => $this->t('Button icon for the @label button is repetitive (button … button). The first button can be omitted.
EmbedTypeBase
  1. Nit: array() -> []
  2. The getDefaultIconUrl() implementation, which serves as the default for several other classes since this is a base class, does not return anything. This violates the interface. That's not just a theoretical problem, but also a practical problem: any class extending EmbedTypeBase that does not override the default implementation of this method will then have broken embed buttons and potentially even fatal PHP errors, unless a custom icon is uploaded. That's far from great.
    I tried to determine why this choice was made. AFAICT it's because it simplifies embed type plugin implementations for testing purposes. But… it then sets the wrong example for all embed type plugins, not just test implementations! So, I think it's better to remove this implementation from this base class and instead let the various test implementations (Aircraft, Animal and EmbedTestDefault) implement this, and preferably at least comply with the interface by returning the empty string.
EmbedTypeInterface
  1. getDefaultIconUrl()'s docs say Use file_create_url() if needed., which is rather mysterious. I clarified it, but it should be checked for correctness of course.
EmbedTypeManager
Nit: missing @see hook_embed_type_plugins_alter()
hook_embed_type_plugins_alter()
Nit: missing @see \Drupal\embed\EmbedType\EmbedTypeManager.
EmbedButtonForm
  1. This still injects the now-deprecated Entity-Manager service. Opened a separate task for that: #2625764: Stop using/injecting the deprecated EntityManager service into EmbedButtonForm.
  2. Nit: array() -> []
  3. Nit: t() -> $this->t() (use StringTranslationTrait; is already happening in FormBase which this extends)
  4. Nit: the docs for updateTypeSettings() used a class name instead of a FQCN.
  5. Button icon image -> Button icon
  6. A form item label says Embed provider its description says Embed type for which this button is to enabled and in case there aren't any, the message No embed type providers found. is shown. This is the only place where the term provider is used. I think this is very confusing. We should be consistent. If we change the label to Embed type, then we can remove the description. And we can clean up the message in case no embed type plugins are present.
EmbedSettingsForm
  1. Nit: array() -> []
  2. Nit: t() -> $this->t() (use StringTranslationTrait; is already happening in FormBase which this extends)
  3. The "stream wrapper manager" service is injected, but not used. Service location is used instead. Easy fix :)
EmbedButtonEditorAccessCheck
  1. ::access() receives a Route $route parameter, but doesn't use it. This can be safely removed. Especially because this has test coverage in \Drupal\embed\Tests\EmbedButtonEditorAccessCheckTest(), and removing it still allows the test to pass.
  2. Several strange bits of text in the docblocks; fixed.
  3. Cache contexts are missing. We'll deal with that in #2567575: Access check missing cacheability metadata (was: "Review necessary changes for use with dynamic_page_cache enabled").
DomHelperTrait.php
Supernit: missing leading backslash in file docblock.
The test embed types Aircraft & Animal
Nit: array() -> []
CommentFileSizeAuthor
#5 interdiff.txt2.26 KBWim Leers
#5 2625822-5.patch22.97 KBWim Leers
#3 interdiff.txt5.9 KBWim Leers
#3 2625822-3.patch24.8 KBWim Leers
#2 2625822-2.patch19.3 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Active » Needs review
FileSize
2625822-2.patch19.3 KB

And patch.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
FileSize
2625822-3.patch24.8 KB
interdiff.txt5.9 KB

Oops, that was incomplete. Here's the rest.

slashrsm’s picture

slashrsm CreditAttribution: slashrsm as a volunteer commented
Status: Needs review » Needs work
Issue tags: +D8Media, +Media Initiative, +Novice

Few comments:

  1. +++ b/src/EmbedButtonInterface.php
@@ -71,6 +71,9 @@ interface EmbedButtonInterface extends ConfigEntityInterface {
+   * If no image file is associated with this Embed Button entity, the embed
+   * type plugin's default icon is used.
+   *

    Should we use "icon" instead of "image"? 

  2. +++ b/src/EmbedButtonListBuilder.php
@@ -7,12 +7,11 @@
+ * Provides a listing of all EmbedButton entities.

    Use space in Embed Button as everywhere else? 

  3. +++ b/src/Form/EmbedButtonForm.php
@@ -28,6 +28,8 @@ class EmbedButtonForm extends EntityForm {
+   *
+   * @todo Don't inject the deprecated entity manager, see https://www.drupal.org/node/2625764.

    It is used only once in this class. Should we just clean this up as part of this patch? 

  4. +++ b/src/Form/EmbedButtonForm.php
@@ -144,18 +145,18 @@ class EmbedButtonForm extends EntityForm {
+      '#description' => $this->t("Icon for the button to be shown in CKEditor toolbar. Leave empty to use the default Entity icon."),

    Single quotes?

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers as a volunteer commented
Status: Needs work » Needs review
FileSize
2625822-5.patch22.97 KB
interdiff.txt2.26 KB
  1. Fixed.
  2. Fixed.
  3. #2625764: Stop using/injecting the deprecated EntityManager service into EmbedButtonForm has a RTBC patch. So, just removed the todo.
  4. Done.
slashrsm’s picture

slashrsm CreditAttribution: slashrsm as a volunteer commented
Status: Needs review » Fixed

Committed. Thank you.

  • slashrsm committed 8327162 on 8.x-1.x authored by Wim Leers 
    Issue #2625822 by Wim Leers, slashrsm: High-level code consistency/best...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.