Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
EmbedInsertCommand
-
- Nit:
Implements …
->@inheritdoc
- Nit:
array()
->[]
- Nit:
EmbedButtonInterface
-
[…] 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.- Punctuation nits on
getTypeSetting()
- It is very confusing to see both
getIconFile()
andgetIconUrl()
: the person reading this interface will wonder why you can't just get the file URL from the return value ofgetIconFile()
. 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
-
- Nit:
array()
->[]
- Nit:
t()
->$this->t()
(neededuse StringTranslationTrait;
for that) - Nit: no blank line between two methods.
- Nit:
EmbedButtonListBuilder
-
- Nit: Unused use statement.
- Nit:
Provides a listing of EmbedButton
is invalid English. - Nit:
array()
->[]
- Nit:
'#alt' => $this->t('Button icon for the @label button
is repetitive (button … button
). The firstbutton
can be omitted.
EmbedTypeBase
-
- Nit:
array()
->[]
- 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 extendingEmbedTypeBase
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
andEmbedTestDefault
) implement this, and preferably at least comply with the interface by returning the empty string.
- Nit:
EmbedTypeInterface
-
getDefaultIconUrl()
's docs sayUse 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
-
- 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. - Nit:
array()
->[]
- Nit:
t()
->$this->t()
(use StringTranslationTrait;
is already happening inFormBase
which this extends) - Nit: the docs for
updateTypeSettings()
used a class name instead of a FQCN. Button icon image
->Button icon
- A form item label says
Embed provider
its description saysEmbed type for which this button is to enabled
and in case there aren't any, the messageNo embed type providers found.
is shown. This is the only place where the termprovider
is used. I think this is very confusing. We should be consistent. If we change the label toEmbed type
, then we can remove the description. And we can clean up the message in case no embed type plugins are present.
- This still injects the now-deprecated
EmbedSettingsForm
-
- Nit:
array()
->[]
- Nit:
t()
->$this->t()
(use StringTranslationTrait;
is already happening inFormBase
which this extends) - The "stream wrapper manager" service is injected, but not used. Service location is used instead. Easy fix :)
- Nit:
EmbedButtonEditorAccessCheck
-
::access()
receives aRoute $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.- Several strange bits of text in the docblocks; fixed.
- 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()
->[]
Comment | File | Size | Author |
---|---|---|---|
#5 | 2625822-5.patch | 22.97 KB | Wim Leers |
Comments
Comment #2
Wim LeersAnd patch.
Comment #3
Wim LeersOops, that was incomplete. Here's the rest.
Comment #4
slashrsm CreditAttribution: slashrsm as a volunteer commentedFew comments:
Should we use "icon" instead of "image"?
Use space in Embed Button as everywhere else?
It is used only once in this class. Should we just clean this up as part of this patch?
Single quotes?
Comment #5
Wim LeersComment #6
slashrsm CreditAttribution: slashrsm as a volunteer commentedCommitted. Thank you.