- Issue created by @Austin986
- 🇹🇭Thailand Nick Hope
@Austin986 What is the reason for opening this as well as https://www.drupal.org/project/media_entity_remote_image/issues/3367202 ✨ No [Add New Remote Image] Form in Media Library modal Closed: duplicate ?
- 🇨🇦Canada Austin986
@Nick Hope, I messed branch, so decided to create fresh one. Gonna close this.
- @austin986 opened merge request.
- Status changed to Needs work
over 1 year ago 6:23am 17 June 2023 - 🇹🇭Thailand Nick Hope
Thank you Austin. It's a good request. Patch #4 applies and works OK for me with Drupal 9.5.9.
I don't think "Type the remote image url above." is necessary below the URL box. If you really wanted then I would prefer "Enter the remote image URL.", but I don't think any text is necessary there.
I wondered whether more info should be added below the URL box, for parity with other core media types, but that would probably just add unnecessary complication. Allowed file types would be useful ("Allowed types: png gif jpg jpeg svg tif tiff.") but it would be best if they are detected so we don't have to remember to keep them in sync if we add more allowed types. I would release this without that.
I think this will need more review than just by me, including some tests in D10.
I ran
phpcs --standard=Drupal
on the patched module and it returned these coding standards issues. I guess they are introduced by the patch, as I eliminated all coding standards issues before the last release:FILE: C:\Users\nick\Sites\ltm-d9\web\modules\contrib\media_entity_remote_image\css\admin.css 4 | ERROR | [x] Expected 1 newline at end of file; 0 found FILE: ...\nick\Sites\ltm-d9\web\modules\contrib\media_entity_remote_image\src\Form\RemoteImageForm.php 105 | ERROR | [x] Inline control structures are not allowed 105 | ERROR | [x] A unary operator statement must not be followed by a space FILE: ...\ltm-d9\web\modules\contrib\media_entity_remote_image\src\Plugin\media\Source\RemoteImage.php 98 | ERROR | [x] Expected 1 blank line after function; 2 found 147 | ERROR | [x] Expected 1 blank line after function; 2 found FILE: ...s\ltm-d9\web\modules\contrib\media_entity_remote_image\src\RemoteImage\RemoteImageFetcher.php 7 | WARNING | [x] Unused use statement 71 | ERROR | [x] Blank comments are not allowed 76 | ERROR | [x] Inline comments must end in full-stops, exclamation marks, question marks, colons, or closing parentheses 94 | ERROR | [x] Expected newline after closing brace 107 | ERROR | [x] Expected 1 blank line after function; 2 found 138 | ERROR | [x] Expected 1 blank line after function; 0 found 139 | ERROR | [x] The closing brace for the class must have an empty line before it FILE: ...\ltm-d9\web\modules\contrib\media_entity_remote_image\src\RemoteImage\RemoteImageResource.php 5 | WARNING | [x] Unused use statement 8 | WARNING | [x] Unused use statement 50 | ERROR | [ ] Parameter $thumbnail_uri is not described in comment 59 | ERROR | [ ] Doc comment for parameter $thumbnailUri does not match actual variable name $thumbnail_uri 69 | ERROR | [ ] Parameter $thumbnail_uri is not described in comment 78 | ERROR | [x] Separate the @param and @return sections by a blank line. 120 | WARNING | [ ] Line exceeds 80 characters; contains 82 characters
- 🇨🇦Canada Austin986
@Nick Hope
I am okay with remove "Type the remote image url above.", as I put it there simply because of easy css applying.
If to remove the description, need add some css fix, so that the button is placed in wanky position. - 🇨🇦Canada Austin986
@Nick Hope
Also, for heads up, I am using this patch for D 10.0.9 in my project.
- 🇱🇺Luxembourg freduni
Hi,
I tried the patch #4 and got the following error when adding a new remote image media with an internal url. (For example the url: /modules/contrib/media_entity_remote_image/images/icons/remote-image.png).
Error: Call to a member function addError() on null in Drupal\media_entity_remote_image\Plugin\media\Source\RemoteImage->getMetadata() (line 116 of modules/contrib/media_entity_remote_image/src/Plugin/media/Source/RemoteImage.php).
The thumbnail generation is disabled in the settings.
- 🇬🇧United Kingdom 2dareis2do
I had to try patch with the diff e.g.
https://git.drupalcode.org/project/media_entity_remote_image/-/merge_req...
Works for me from both media widget and ckeditor media icon. I would expect this functionality as part of this module.
- Status changed to Needs review
11 months ago 3:19pm 17 January 2024 - Status changed to Needs work
11 months ago 4:07pm 17 January 2024 - 🇬🇧United Kingdom 2dareis2do
A lot of those issues PHPCS errors can be fixed by using PHPCBF.
These one probably require a bit more attention.
e.g.
phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml media_entity_remote_image/ FILE: ...contrib/media_entity_remote_image/media_entity_remote_image.module ------------------------------------------------------------------------ FOUND 1 ERROR AFFECTING 1 LINE ------------------------------------------------------------------------ 49 | ERROR | All functions defined in a module file must be prefixed | | with the module's name, found "_force_update_media_name" | | but expected | | "media_entity_remote_image__force_update_media_name" ------------------------------------------------------------------------ FILE: ...b/media_entity_remote_image/src/RemoteImage/RemoteImageFetcher.php ------------------------------------------------------------------------ FOUND 0 ERRORS AND 5 WARNINGS AFFECTING 5 LINES ------------------------------------------------------------------------ 61 | WARNING | \Drupal calls should be avoided in classes, use | | dependency injection instead 80 | WARNING | \Drupal calls should be avoided in classes, use | | dependency injection instead 82 | WARNING | \Drupal calls should be avoided in classes, use | | dependency injection instead 88 | WARNING | \Drupal calls should be avoided in classes, use | | dependency injection instead 89 | WARNING | \Drupal calls should be avoided in classes, use | | dependency injection instead ------------------------------------------------------------------------ FILE: .../media_entity_remote_image/src/RemoteImage/RemoteImageResource.php ------------------------------------------------------------------------ FOUND 3 ERRORS AND 1 WARNING AFFECTING 4 LINES ------------------------------------------------------------------------ 48 | ERROR | Parameter $thumbnail_uri is not described in comment 57 | ERROR | Doc comment for parameter $thumbnailUri does not match | | actual variable name $thumbnail_uri 67 | ERROR | Parameter $thumbnail_uri is not described in comment 119 | WARNING | Line exceeds 80 characters; contains 82 characters ------------------------------------------------------------------------ FILE: ..._entity_remote_image/src/Plugin/Field/FieldType/RemoteImageUrl.php ------------------------------------------------------------------------ FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES ------------------------------------------------------------------------ 83 | WARNING | t() calls should be avoided in classes, use | | \Drupal\Core\StringTranslation\StringTranslationTrait | | and $this->t() instead 85 | WARNING | t() calls should be avoided in classes, use | | \Drupal\Core\StringTranslation\StringTranslationTrait | | and $this->t() instead ------------------------------------------------------------------------
certainly the $thumbnail_uri seems like an easy fix.