🇸🇮Slovenia @slogar32

Entity browser behaves like this by default. If you add a MediaImageUpload instance of the browser on a FileBrowserWidget, the same error is given. Since there is no handling of this in the module, that we extend I am closing this.

Solved in https://git.drupalcode.org/project/media_pixabay/-/commit/b6e357c91825f9....

slogar32 → created an issue.

slogar32 → created an issue.

slogar32 → created an issue.

slogar32 → created an issue.

slogar32 → created an issue.

I fixed the issue above, thank you for your contribution!

This is not available as metadata. It is stored in the CDN media type source field in the database (field_media_kontainer_cdn) and is used for BE logic for the actual creation of the media entity on import and for CDN templates/responsive image logic. And since originally, this was not even a media type (CDN mode), but only available as an extended link field (which is now the source field of the CDN media type), this should be considered as a feature request and not a bug report. If you are ok with that, I will change this issue type accordingly.

Code looks good, and the new UI look also, please just fix the Pixabay search bar, so it automatically fits the modal of the entity:browser.

slogar32 → made their first commit to this issue’s fork.

Hello!

I've assigned credit to this issue, according to How is credit granted for Drupal core issues → . @admirlju got credit because he created the initial solution, @lennart got credit, because of a useful original issue report (see first two bullet points here https://www.drupal.org/about/core/policies/maintainers/how-is-credit-granted-for-drupal-core-issues#receive-credit → ).

I did not grant you credit because of bullet point 7, 2nd level bullet point 3 here https://www.drupal.org/about/core/policies/maintainers/how-is-credit-granted-for-drupal-core-issues#not-receive-credit → and @ankithashetty, because she did copy paste of my code from the comment, and she also missed some obvious things of the same matter from her copy paste fixes, that I had to fix myself later on (could fall under bullet point number 9 from the last link).

So basically I had to re-test this whole solution again, and I found further issues, that I also fixed myself. And because of the described things above, I do not see you eligible for credit here, sorry.

Best regards,
Domen

Thank you both for your contributions!

slogar32 → made their first commit to this issue’s fork.

I tested this with multiple widgets (with each having a different media type with an image source plugin selected). I had to add some edge cases and fix the code quality slightly. I've also updated the readme file. Thank you for your contributions.

Thank you so much for the explanations! I can say, that I learned something new and useful from this application :)

Thank you again for your time!

Tested this locally with multiple modules as dependencies on Kontainer media types and merged it to the 1.x branch. Will be added to the 1.1.2 release. Thank you again!

I have now pushed the changes for points 1. and 2. from the above list, and explained number 4. in my above comment. I understand, that those were the 3 security issues that were mandatory to fix. If I need to do something on point 3 from the list, please read my above questions and point me in the right direction. Otherwise, I think this is ready for review now.

Thanks for the bug report and the patch! I will create a merge request with the changes and include it in the next release.

Thank you very much for the review!

I have some questions/information about your points:

4. I didn't put any access requirements to the route definition in the routing.yml file, because I check this dynamically in code (see checkAccess function in KontainerService.php, which is called in the createEntities function in the controller for this route). I did this, because the media type is fetched dynamically from the JSON response each time from the Kontainer DAM, or decided based on the module configurations (if CDN is set as media source, then it is set to this value which is not in the response from the JSON.) So I would leave this as it is, if you do not see it as an access bypass vulnerability, based on this explanation.

3. I am not exactly sure what I need to do here. I am generating the CSRF token based on the route path, then I add this token to the ajaxTrustedUrl drupalSettings, where the token is added as a query parameter to the media path only (and not the URL), and I also add It to custom drupalSettings. Then in the Javascript library, where the actual call to the route is made ( url: Drupal.url(url) + '?token=' + drupalSettings.kontainer.token,) I build the URL from the values, that are generated in the widget class. I was doing this based on this example here: https://drupal.stackexchange.com/questions/221251/generating-url-with-csrf-token

So what I see that I could change here is to actually generate the URL object in the widget class by adding the CSRF token as the URL query parameter directly to the object, and then add this URL object to the drupalSettings, instead of building the URL in the Javascript library with Drupal.url(). But the CSRF token generation would stay as it is. Is this the direction, that you are pointing me to? If not, could you please provide me an explanation or an example, that would lead me in the right direction? Thanks!

Points 1 and 2 are clear to me. I will push those fixes by the end of this week, but I would again kindly ask you to provide some information on the above two points, when you have some time (I am putting the application back to Needs review, so you see my questions, even though I didn't push any changes yet).

Thank you again for the feedback!

i've posted an answer on Drupal Answers for a workaround, until this issue is fixed -> https://drupal.stackexchange.com/a/318397/104069

slogar32 → created an issue.

slogar32 → created an issue.

slogar32 → created an issue.

Thanks for the extra review! Yeah, this is in the JS file, but you can't write FALSE, TRUE and NULL upper case in JS, so this Drupal standard should be ignored for the Javascript file, DrupalPractice doesn't return any errors for it also.

Those are suggestions for better code, but I think numbers 1 and 2 are needed to pass this validation here, since those are direct Drupal standards (those two are quick fixes also). Depends on the people who have roles to confirm you for the opt-in validation. Good luck in any case!

Hey, following is my module review:

1. use Drupal\Core\Render\ElementInfo; This class is not found neither on latest Drupal 9 and on latest Drupal 10 releases. This will results in a PHP error, since you then injext this into the class in the formatted. You also have the constructor function define twice. In both of those constructors the parent constructor is not called.
   

     /**
      * ElementInfo This property stores information about the element.
      *
      * @var mixed
      */
     protected $elementInfo;
   
     /**
      * MyCustomClass constructor.
      *
      * @param \Drupal\Core\Render\ElementInfo $elementInfo
      *   The element info service.
      */
     public function __construct(ElementInfo $elementInfo) {
       $this->elementInfo = $elementInfo;
     }
   
     /**
      * The element info manager.
      *
      * @var \Drupal\Core\Render\ElementInfoManagerInterface
      */
     protected $elementInfo;
   
     /**
      * Constructs a TextSummaryOnlyFormatter object.
      *
      * @param \Drupal\Core\Render\ElementInfoManagerInterface $elementInfo
      *   The element info manager.
      */
     public function __construct(ElementInfoManagerInterface $elementInfo) {
       $this->elementInfo = $elementInfo;
     }

2. The create function is also not compatible with the parent class
   

     /**
      * {@inheritdoc}
      */
     public static function create(ContainerInterface $container, $plugin_id, $plugin_definition, array $context) {
       return new static(
         $container->get('element_info')
       );
     }

3. There is already a contrib module which does basically the same thing. The only difference, that I see here, that you added some extra logic with the trim summary length. -> Text Summary Formatter →
4. I also don't think, that there is enough code to for security opt-in approval in this module, see https://groups.drupal.org/node/195848. This is debatable though.

Hey, here is my review of the module:

1. FILE: /home/domen/Documents/drupal10/web/modules/contrib/url_text/src/Plugin/Field/FieldWidget/UrlTextfieldWidget.php
   ---------------------------------------------------------------------------------------------------------------------
   FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
   ---------------------------------------------------------------------------------------------------------------------
    39 | WARNING | There must be no blank line following an inline comment
   ---------------------------------------------------------------------------------------------------------------------
   
   Time: 49ms; Memory: 8MB
   
   domen@domen-XPS-13-9370:~/Documents/drupal10$ vendor/bin/phpcs web/modules/contrib/url_text/ --standard=Drupal
   
   FILE: /home/domen/Documents/drupal10/web/modules/contrib/url_text/src/Plugin/Field/FieldWidget/UrlTextfieldWidget.php
   ---------------------------------------------------------------------------------------------------------------------
   FOUND 14 ERRORS AND 1 WARNING AFFECTING 11 LINES
   ---------------------------------------------------------------------------------------------------------------------
     8 | WARNING | [x] Unused use statement
    30 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 0
    30 | ERROR   | [x] 2 spaces found before inline comment; expected "// /**" but found "//  /**"
    31 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 0
    31 | ERROR   | [ ] Comment indentation error, expected only 2 spaces
    32 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 0
    33 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 0
    34 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 0
    34 | ERROR   | [ ] Comment indentation error, expected only 2 spaces
    35 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 0
    35 | ERROR   | [ ] Comment indentation error, expected only 4 spaces
    36 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 0
    37 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 0
    38 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 0
    39 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 0
   ---------------------------------------------------------------------------------------------------------------------
   PHPCBF CAN FIX THE 12 MARKED SNIFF VIOLATIONS AUTOMATICALLY
   ---------------------------------------------------------------------------------------------------------------------
   
   
   FILE: /home/domen/Documents/drupal10/web/modules/contrib/url_text/src/Plugin/Field/FieldType/UrlItem.php
   --------------------------------------------------------------------------------------------------------
   FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
   --------------------------------------------------------------------------------------------------------
    5 | WARNING | [x] Unused use statement
    6 | WARNING | [x] Unused use statement
   --------------------------------------------------------------------------------------------------------
   PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
   --------------------------------------------------------------------------------------------------------
   

   This is the output from

   phpcs web/modules/contrib/url_text/ --standard=DrupalPractice
   

   , please fix this.

2. In UrlTextfieldWidget.php there is commented out code, I think that should be removed:
   

   //  /**
   //   * {@inheritdoc}
   //   */
   //  public static function defaultSettings(): array {
   //    return [
   //      'size' => 60,
   //      'placeholder' => '',
   //      'url_schemes' => ['http', 'https'],
   //    ] + parent::defaultSettings();
   //  }

3. By some functions the return type is defined and by some it isn't, the return type should be defined consistently. For example in UrlTextfieldWidget.php:
   

     /**
      * Generate the URL text field description based on allowed schemes.
      */
     private function createDescription(): string {
   
       return implode(', ', UrlTextfieldWidget::$schemes);
     }
   
     /**
      * Retrieve the allowed schemes from the field settings.
      */
     private function getSchemes(FieldItemListInterface $items) {
       $def = $items->getFieldDefinition();
       $schemes = $def->getSetting('allowed_schemes');
   
       $enabled = [];
       // Iterate the allowed schemes to return those selected.
       foreach ($schemes as $scheme) {
         if ($scheme) {
           $enabled[] = $scheme;
         }
       }
   
       return $enabled;
     }

4. In UrlItem.php you have an unused local variable defined, this can probably be removed:
   

     /**
      * {@inheritdoc}
      */
     public static function propertyDefinitions(FieldStorageDefinitionInterface $field_definition): array {
       $properties['value'] = DataDefinition::create('string')
         ->setLabel(new TranslatableMarkup('Value'));
   
       return [];
     }
   

5. The readme file is a bit vague, please read through Module documentation guidelines → and update it.
6. The url_text.schema.yml file should be in the /config/schema directory.
7. In UrlItem.php where do you get the setting values from in the schema function (type, length and binary), you extend the FieldItemBaseclass. Did you extend another field type before and then forgot to change this?

.
Don't forget to run phpcs with Drupal and DrupalPractice code standards after your fixes as well.

I reviewed the module code, here are my findings:

1. FILE: testsuite.module
   

   /**
    * @file
    * This module provides tests that can be ran from the admin.
    */

   The description for a module is Hook implementations for the [module name] module. where [module name] is the module name given in the .info.yml file.

2. The documentation comment for constructors is not mandatory anymore, If it is given, the description must be Constructs a new [class name] object. where [class name] includes the class namespace.
   

     /**
      * Constructs a new TestSuiteController.
      *
      * @param object $loadResourceService
      *   Drupal\testsuite\LoadResourceService.
      * @param object $testMenuService
      *   Drupal\testsuite\TestMenuService.
      * @param object $shellExecService
      *   Drupal\testsuite\ShellExecService.
      */
     public function __construct(
       LoadResourceService $loadResourceService,
       TestMenuService $testMenuService,
       ShellExecService $shellExecService
     ) {
       $this->testMenuService = $testMenuService;
       $this->loadResourceService = $loadResourceService;
       $this->shellExecService = $shellExecService;
     }

   Function and method declarations are written on a single line.
   So the above example should be something like:

   
     public function __construct(LoadResourceService $loadResourceService, TestMenuService $testMenuService, ShellExecService $shellExecService) {
       $this->testMenuService = $testMenuService;
       $this->loadResourceService = $loadResourceService;
       $this->shellExecService = $shellExecService;
     }

   If you want to keep the documentation comment with the corrected format. Don't forget to do this for all the constructors in the module.

3. I would also suggest that you don't use object for the @param descriptions in documentation comments, but instead the actual class/type of the parameter
   

     /**
      * Builds the report for custom tests.
      *
      * @param object $request
      *   Symfony\Component\HttpFoundation\Request.
      *
      * @return array
      *   The page as a render array.
      */
     public function customReport(Request $request)

   should be

     /**
      * Builds the report for custom tests.
      *
      * @param \Symfony\Component\HttpFoundation\Request $request
      *    The incoming HTTP request object.
      *
      * @return array
      *     The page as a render array.
      */
     public function customReport(Request $request)

   It then makes sense for the @param description not to be the actual namescape with the class name, but an actual description (doesn't need to be same as in the example that I gave above).
   You have it like this in the TestInterface.php, but you are missing the \ in front of the class with the namespace in the @param comment, it should be like this::

     /**
      * Runs the test and returns if the test succeeded or failed.
      *
      * @param \Symfony\Component\HttpFoundation\Request $request
      *   The request object.
      *
      * @return string
      *   Success or Test Passed if the test succeeded.
      *   Failed and the eroors if the test failed.
      */
     public function runTest(Request $request);

   Please check, that all of your @param in documentation comments are consistent, another example that should be fixed here in GuzzleService.php:

      /**
      * GuzzleService constructor.
      *
      * @param \Guzzle\ClientInterface $httpClient
      *   The guzzle client.
      */
     public function __construct(ClientInterface $httpClient) {
       $this->httpClient = $httpClient;
     }
   

   Should be:

     /**
      * Constructs a new GuzzleService object.
      *
      * @param \GuzzleHttp\ClientInterface $httpClient
      *   The guzzle client.
      */
     public function __construct(ClientInterface $httpClient) {
       $this->httpClient = $httpClient;
     }

4. The functions should have parameters type declarations and return type declarations. Somewhere you have the @return tag in the documentation comment and somewhere not, that should also be consistent (I would add @return tags everywhere, except for void returning functions, it is not needed there)
   

     /**
      * Builds the report for phpunit tests that are accessable through links.
      *
      * @param string $area
      *   The area of the module. Core, contrib or custom.
      * @param string $module
      *   The module.
      * @param string $directory
      *   The phpunit test directory. Unit, Functional, Kernel etc.
      * @param string $file
      *   The phpunit test file name.
      *
      * @return array
      *   The page as a render array.
      */
     public function phpUnitFileReport($area, $module, $directory, $file = NULL)

   should be:

     /**
      * Builds the report for phpunit tests that are accessable through links.
      *
      * @param string $area
      *   The area of the module. Core, contrib or custom.
      * @param string $module
      *   The module.
      * @param string $directory
      *   The phpunit test directory. Unit, Functional, Kernel etc.
      * @param null|string $file
      *   The phpunit test file name.
      *
      * @return array
      *   The page as a render array.
      */
     public function phpUnitFileReport(string $area, string $module, string $directory, ?string $file = NULL): array

5. There are also some code duplications, you should extract that as a function, for example: in TestSuiteController.php lines 281-300 are duplicated in lines 187-206 and lines 303-317 are duplicated in 209-223, both of these duplications could be one function. Also in ConfigurationForm.php, you have the same line twice in lines 351 and 352.
6. Class properties should have type declarations. Those should be interfaces, where possible. Also when they are injected in the class, in the documentation comment @param type, and in the actual constructor function parameter type declaration it should be an interface where possible. For example:
   

     /**
      * Drupal\Core\Messenger\Messenger.
      *
      * @var object
      */
     protected $messenger;
   
     /**
      * The path to the phpunit.xml.dist file.
      *
      * @var string
      */
     protected $phpunitXmlDistPath;

   should be:

     /**
      * The messenger service.
      *
      * @var \Drupal\Core\Messenger\MessengerInterface
      */
     protected MessengerInterface $messenger;
   
     /**
      * The path to the phpunit.xml.dist file.
      *
      * @var string
      */
     protected string $phpunitXmlDistPath;

   and in the constructor:

     /**
      * Initializer Service constructor.
      */
     public function __construct(MessengerInterface $messenger) {
       $this->messenger = $messenger;
       if ($this->isWindows()) {
         $this->phpunitCore = dirname(dirname(dirname(dirname(__DIR__)))) . '\core';
         $this->simpleTestPath = dirname(dirname(dirname(dirname(__DIR__)))) . '\sites\simpletest';
         $this->phpunitXmlDistPath = dirname(dirname(dirname(dirname(__DIR__)))) . '\core\phpunit.xml.dist';
         $this->phpunitXmlPath = dirname(dirname(dirname(dirname(__DIR__)))) . '\core\phpunit.xml';
       }
       else {
         $this->phpunitCore = dirname(dirname(dirname(dirname(__DIR__)))) . '/core';
         $this->simpleTestPath = dirname(dirname(dirname(dirname(__DIR__)))) . '/sites/simpletest';
         $this->phpunitXmlDistPath = dirname(dirname(dirname(dirname(__DIR__)))) . '/core/phpunit.xml.dist';
         $this->phpunitXmlPath = dirname(dirname(dirname(dirname(__DIR__)))) . '/core/phpunit.xml';
       }
     }

   . And when I just posted this example, you can probably replace the dirname nesting with the

   DIRECTORY_SEPARATOR

   constant, if you used this because of multi OS support, then just use the levels parameter of the dirname function instead of nesting it like this.

7. Services with public methods should have interfaces implemented. When those classes are used in dependency injection, use the interface.
8. There are some typos in your documentation comments like "Builds the report for phpunit tests that are accessable through links." But this is not critical at all, just pointing it out, because it is a quick fix (should be accessible instead of accessable).

This is a full review of the code, when all of this is fixed I see no reason why this shouldn't be in the RBTC status. Please fix all of the occurrences of the issues mentioned above, I've just put some examples in the list. After you do this run phpcs again for Drupal and DrupalPractice standards. If you have any questions or need some help with the fixes just contact me directly on one of the platforms on my profile.

slogar32 → created an issue.

I am changing the issue priority as per issue priorities → .

slogar32 → created an issue.

slogar32 → created an issue.

slogar32 → created an issue.

Both branches have been deleted.

Thank you for your feedback!

I have fixed everything from your list of findings. The changes have been pushed to the 1.x branch. If there is something more to do, please let me know.

Hey! Thank you for the feature implementation. I quickly went through the changes, could you please:

• -Make the pixabay URL field map available on all media types with the image source plugin (since the module supports adding of those on the entity browser widget). Use
  

  $sourcePlugin = $entity->getSource();
  if (!$sourcePlugin instanceof (Drupal\media\Plugin\media\Source\)Image) {
    return;
  }

  instead of

  if ($entity->id() != 'image') {
    return;
  }

  in the form alter hook, that you've implemented.

• -Use camelCase for the newly defined $pixabay_url_field_name variable in the Pixabay plugin class please (I know $form_state is snake case, but we used camel case throughout the whole class, so it should stay consistent).
• -Run hpcs --standard=Drupal,>DrupalPractice to see if your code follows Drupal coding standards. Add the "." to the end of the inline comments in the form alter hook, that's what I spotted with my eyes, maybe there's more to fix.

Otherwise, this looks good!

slogar32 → created an issue.

Thanks for the nice feedback. If the Pixabay API supports it, It's possible, of course. But as I am currently very busy in my private life, I don't see it happening anytime soon, if I would have to implement it, sorry. Maybe someone from the open source community will see this feature request and jump in, hopefully.