[1.x] URL Text

Thank you for applying!

Please read Review process for security advisory coverage: What to expect → for more details and Security advisory coverage application checklist → to understand what reviewers look for. Tips for ensuring a smooth review → gives some hints for a smoother review.

The important notes are the following.

• If you have not done it yet, you should run phpcs --standard=Drupal,DrupalPractice on the project, which alone fixes most of what reviewers would report.
• For the time this application is open, only your commits are allowed.
• The purpose of this application is giving you a new drupal.org role that allows you to opt projects into security advisory coverage, either projects you already created, or projects you will create. The project status won't be changed by this application and no other user will be able to opt projects into security advisory policy.
• We only accept an application per user. If you change your mind about the project to use for this application, or it is necessary to use a different project for the application, please update the issue summary with the link to the correct project and the issue title with the project name and the branch to review.

To the reviewers

Please read How to review security advisory coverage applications → , Application workflow → , What to cover in an application review → , and Tools to use for reviews → .

The important notes are the following.

• It is preferable to wait for a Code Review Administrator before commenting on newly created applications. Code Review Administrators will do some preliminary checks that are necessary before any change on the project files is suggested.
• Reviewers should show the output of a CLI tool → only once per application.
• It may be best to have the applicant fix things before further review.

For new reviewers, I would also suggest to first read In which way the issue queue for coverage applications is different from other project queues → .

I did not make a complete review, but I wanted to report that src/Plugin/Field/FieldType/IntegerItem.bak.php is not a correct filename for a file containing a PHP class. The namespace used for the class is one that should be used by Drupal core; the class itself seems a copy of the class already implemented by Drupal core.

Hi. Yes, I believe the problem is that you are looking at the wrong branch. It should be 1.x.

fwiw, you are correct that on the branch you looked at that is a duplicate of the core class. It was named .bak because it wasn't to be used there, it was only copied into the folder to make it easier to refer to.

Fix phpcs issues.

phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml url_text

FILE: url_text/src/Plugin/Field/FieldWidget/UrlTextfieldWidget.php
--------------------------------------------------------------------------------
FOUND 9 ERRORS AND 3 WARNINGS AFFECTING 9 LINES
--------------------------------------------------------------------------------
  22 | ERROR   | [ ] Missing member variable doc comment
  83 | WARNING | [ ] #description values usually have to run through t() for translation
  84 | ERROR   | [x] There should be no white space after an opening "["
  84 | ERROR   | [x] There should be no white space before a closing "]"
  99 | ERROR   | [x] Inline comments must end in full-stops, exclamation marks, question marks, colons, or closing parentheses
 101 | ERROR   | [x] Inline comments must end in full-stops, exclamation marks, question marks, colons, or closing parentheses
 105 | WARNING | [ ] Translatable strings must not begin or end with white spaces, use placeholders with t() for variables
 111 | WARNING | [ ] Translatable strings must not begin or end with white spaces, use placeholders with t() for variables
 111 | ERROR   | [ ] Concatenating translatable strings is not allowed, use placeholders instead and only one string literal
 122 | ERROR   | [x] Expected one space after the comma, 0 found
 123 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 1
 123 | ERROR   | [x] Closing brace indented incorrectly; expected 2 spaces, found 1
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 7 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------


FILE: url_text/src/Plugin/Field/FieldFormatter/UrlFormatter.php
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
 6 | ERROR | [x] Use statements should be sorted alphabetically. The first wrong one is Drupal\Core\Field\FieldItemListInterface.
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------


FILE: url_text/src/Plugin/Field/FieldType/UrlItem.php
--------------------------------------------------------------------------------
FOUND 10 ERRORS AFFECTING 8 LINES
--------------------------------------------------------------------------------
   9 | ERROR | [x] Use statements should be sorted alphabetically. The first wrong one is Drupal\Core\Field\FieldItemBase.
  55 | ERROR | [x] Expected one space after the comma, 0 found
  67 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 1
  67 | ERROR | [x] Missing function doc comment
  69 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 3
  70 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 1
  70 | ERROR | [x] Expected 1 blank line after function; 0 found
 157 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 3
 158 | ERROR | [x] Expected 4 space(s) before asterisk; 3 found
 159 | ERROR | [x] Expected 4 space(s) before asterisk; 3 found
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 10 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------

Thanks Vishal. That exposed that PhpStorm wasn't fully connecting to PHPCS. All items were addressed.

FILE: src/Plugin/Field/FieldWidget/UrlTextfieldWidget.php

'#description' => 'Enter a URL. Allowed schemes are: ' . $this->createDescription() . '.',

#description values have to run through t() for translation.

Corrected.

Hi. Anything I can do to move this along?

Remember to change the status to Needs review when the project is ready for review.

• The following points are just a start and don't necessarily encompass all of the changes that may be necessary
• A specific point may just be an example and may apply in other places
• A review is about code that doesn't follow the coding standards, contains possible security issue, or doesn't correctly use the Drupal API; the single points aren't ordered, not even by importance

src/Plugin/Field/FieldWidget/UrlTextfieldWidget.php

        if (!in_array($scheme_type, $this->schemes)) {
          $form_state->setError($element, $scheme_type . ' ' . $this->t('is not an enabled URL scheme.'));
        }

        if (!$preg_match) {
          $form_state->setError($element, $this->t('Invalid format for') . ' ' . $scheme);
        }

The code passes to $this->t() a literal string, and that is perfect, but instead of concatenating a translated string with another string, it needs to use t()-placeholders. It helps translators who can find a better translation.

  public function formElement(FieldItemListInterface $items, $delta, array $element, array &$form, FormStateInterface $form_state): array {
    $this->schemes = $this->getSchemes($items);

    $element = [
      '#title' => $this->t('URL'),
      '#type' => 'textfield',
      '#default_value' => $items[$delta]->value ?? NULL,
      '#size' => $this->getSetting('size'),
      '#placeholder' => $this->getSetting('placeholder'),
      '#maxlength' => $this->getFieldSetting('max_length'),
      '#attributes' => ['class' => ['url-text']],
      '#description' => $this->t('Enter a URL. Allowed schemes are: @desc.', ['@desc' => $this->createDescription()]),
      '#element_validate' => [[$this, 'validate']],
    ];

    return ['value' => $element];
  }

Since $element is passed as argument to that method, that method should add keys to that array, as Drupal core widget classes do, not remove all the existing keys, as $element = [ /* omissis */ ]; does.
The value returned is $element.

I cannot say if there is any technical reason, but any validation, submission, or AJAX callback a Drupal core widget class adds is a static method, as in the following code.

  $element['media_library_update_widget'] = [
    '#type' => 'submit',
    '#value' => $this->t('Update widget'),
    '#name' => $field_name . '-media-library-update' . $id_suffix,
    '#ajax' => [
      'callback' => [static::class, 'updateWidget'],
      'wrapper' => $wrapper_id,
      'progress' => [
        'type' => 'throbber',
        'message' => $this->t('Adding selection.'),
      ],
    ],
    '#attributes' => [
      'data-media-library-widget-update' => $field_widget_id,
      'class' => ['js-hide'],
    ],
    '#validate' => [[static::class, 'validateItems']],
    '#submit' => [[static::class, 'addItems']],
    // We need to prevent the widget from being validated when no media items
    // are selected. When a media field is added in a subform, entity
    // validation is triggered in EntityFormDisplay::validateFormValues().
    // Since the media item is not added to the form yet, this triggers errors
    // for required media fields.
    '#limit_validation_errors' => !empty($referenced_entities) ? $limit_validation_errors : [],
  ];

I would rather stay on the safe side and use only static methods for those callbacks.

src/Plugin/Field/FieldType/UrlItem.php

    $random = new Random();
    $max_length = $field_definition->getSetting('max_length');

    // When the maximum length is less than 15, or the field needs to be unique,
    // generate a random word using the maximum length.
    if ($max_length <= 15 || $field_definition->getConstraint('UniqueField')) {
      $values['value'] = ucfirst($random->word($max_length));

      return $values;
    }

    // The minimum length is either 10% of the maximum length, or 15 characters
    // long, whichever is greater.
    $min_length = max(ceil($max_length * 0.10), 15);

    // Reduce the max length to allow us to add a period.
    $max_length -= 1;

    // The random value is generated multiple times to create a slight
    // preference towards values that are closer to the minimum length of the
    // string. For values larger than 255 (which is the default maximum value),
    // the bias towards minimum length is increased. This is because the default
    // maximum length of 255 is often used for fields that include shorter
    // values (i.e. title).
    $length = mt_rand($min_length, mt_rand($min_length, $max_length >= 255 ? mt_rand($min_length, $max_length) : $max_length));

    $string = $random->sentences(1);
    while (mb_strlen($string) < $length) {
      $string .= " {$random->sentences(1)}";
    }

    if (mb_strlen($string) > $max_length) {
      $string = substr($string, 0, $length);
      $string = substr($string, 0, strrpos($string, ' '));
    }

    $string = rtrim($string, ' .');

    // Ensure that the string ends with a full stop if there are multiple
    // sentences.
    $values['value'] = $string . (str_contains($string, '.') ? '.' : '');

    return $values;

Since the field is for URLs, the sample values must be values the field handles/accepts, not a concatenation of random words with periods.

Changes made.

• validate is now called statically
• the generation of a default value for the field item form was removed as the default value is static
• handling of the element variable in formfield is now standard

generateSampleValue() is not used to generate the default value for a form element.

I apologize if I was not clear: I did not mean that generateSampleValue() must be removed, but that it needs to generate random values that are still acceptable for that field. See for example:

BooleanItem::generateSampleValue(), which generates a random value that is either 0 or 1
EmailItem::generateSampleValue(), which generates an email address with a random local-part and example.com as domain.
TelephoneItem::generateSampleValue(), which generates a random telephone number between 100000000 and 999999999

As long as there is a random part and the value still passes the field validation, the value is acceptable.

I’m confused. It’s in FieldItem that the random value is generated, which is the field settings form, not the URL field value itself in the FieldWidget.

In the field settings form are the checkboxes to select the allowed schemes. By default http and https are set. What would need a random generation?

Hi. This seems to be stuck. Are you waiting for me to put back a method to generate a random value? Again, it’s not a required method, but if I need to put it back in order to finish this process please tell me.

Raising the priority a notch to raise it a bit
in the stack as it’s been a couple week.

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.

• Straightened out the storage schema and settings schema
• Fixed the PHPCS issues and made sure no new ones were introduced
• Replaced the placeholder README.txt with a complete README.md that adheres to best practices.

This issue seems stuck. Can we un-stick it, please?

I'm reviewing

I'd be great to find a more meaningful module name:

https://git.drupalcode.org/project/url_text/-/blob/1.x/url_text.info.yml...

name: URL

Same here:
https://git.drupalcode.org/project/url_text/-/blob/1.x/src/Plugin/Field/...

 *   label = @Translation("URL"),

The mailto format doesn't seem to be correct:
https://en.wikipedia.org/wiki/Mailto
https://git.drupalcode.org/project/url_text/-/blob/1.x/src/Plugin/Field/...

      'mailto' => 'mailto://',

I've tested the module and honestly I don't see any real difference with the plain text fields:

There is no difference with plain text. It is a plain text field. The module validates the format when entering the URL. It has nothing to do with the rendering. If you, for example, enter https://z it will present an error when you attempt to save because the format is wrong. That is all the module does.

As to the name, URL is as meaningful as Phone. It is simply the type of field.

It would seem there are no technical issues remaining. If so, can we move on to the final step?

Hm, it looks like your account is already approved as git vetted user to make full projects. I could not find another project application under your username, so a drupal.org administrator has probably set that already for you?

Anyway, the code looks good.

Thanks for your contribution, Jeff!

Your account is already approved so you can opt into security advisory coverage.

Here are some recommended readings to help with excellent maintainership:

• Dries → ' post on Responsible maintainers
• Best practices for creating and maintaining projects →
• Maintaining a drupal.org project with Git →
• Commit messages - providing history and credit →
• Release naming conventions → .
• Helping maintainers in the issue queues →

You can find lots more contributors chatting on Slack → or IRC → in #drupal-contribute. So, come hang out and stay involved → !

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review → . I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

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