Add constraints to the linky link field based on settings

Looking good, some cleanup we can do to reduce the complexity of the contraint validator by a fair bit.
In terms of testing, we should be able to test the validation with a Kernel test, which would just leave testing the form.
Here's an example of a kernel test for a validation plugin
In terms of testing the form it would be good to test
a) that the route can't be seen without the permission
b) that the form can be submitted and the new values are saved

1. +++ b/config/schema/linky.schema.yml
   @@ -15,3 +15,18 @@ field.formatter.settings.linky_label:
   \ No newline at end of file
   
   +++ b/linky.install
   @@ -38,3 +38,15 @@ function linky_update_8101() {
   \ No newline at end of file
   
   +++ b/linky.links.menu.yml
   @@ -11,3 +11,9 @@ entity.linky.admin:
   \ No newline at end of file
   
   +++ b/linky.routing.yml
   @@ -6,3 +6,10 @@ entity.linky.admin:
   \ No newline at end of file
   

   Some white space issues here, can you set your IDE settings

2. +++ b/linky.module
   @@ -87,3 +89,25 @@ function linky_entity_predelete(EntityInterface $entity) {
   +function _linky_get_settings(): ImmutableConfig {
   +    return \Drupal::config('linky.settings');
   

   Do we need this?

3. +++ b/linky.permissions.yml
   @@ -20,3 +20,7 @@ edit own linky entities:
   +  restrict access: true
   

   I don't think there's a security implication here, so we can probably drop this

4. +++ b/src/Form/LinkySettingsForm.php
   @@ -0,0 +1,83 @@
   +      '#type' => 'fieldset',
   

   we could use the built in #checkboxes here.

   You can still set the #description etc

5. +++ b/src/Form/LinkySettingsForm.php
   @@ -0,0 +1,83 @@
   +    $form_state->cleanValues();
   

   never seen this used before - what's it for?

6. +++ b/src/Form/LinkySettingsForm.php
   @@ -0,0 +1,83 @@
   +    Cache::invalidateTags(['entity_field_info']);
   

   👌

7. +++ b/src/Plugin/Validation/Constraint/LinkyLinkConstraintValidator.php
   @@ -0,0 +1,65 @@
   +    if (!$validated) {
   

   this would always be FALSE here right?

8. +++ b/src/Plugin/Validation/Constraint/LinkyLinkConstraintValidator.php
   @@ -0,0 +1,65 @@
   +      $has_tel = $validated = (bool) preg_match('/^tel:[0-9\ ]+/i', $uri);
   

   this could also include +, ( and -

   would it be simpler to just use parse_url($uri, PHP_URL_SCHEME) === 'tel' and side-step all the myriad of phone numbering schemes across the world - browsers don't ship a default pattern attribute for <input type="tel"> because its in the too hard basket, so we should too

9. +++ b/src/Plugin/Validation/Constraint/LinkyLinkConstraintValidator.php
   @@ -0,0 +1,65 @@
   +      if (empty($constraint->settings['telephone'])) {
   +        if ($has_tel) {
   

   we can combine these into a single if

10. +++ b/src/Plugin/Validation/Constraint/LinkyLinkConstraintValidator.php
    @@ -0,0 +1,65 @@
    +          $this->context->buildViolation($constraint->notAllowedMessage)->setParameters(['@uri' => $uri])->atPath('0.uri')->addViolation();
    ...
    +          $this->context->buildViolation($constraint->notAllowedMessage)->setParameters(['@uri' => $uri])->atPath('0.uri')->addViolation();
    

    at these two points, we can return, because if the url is a tel link, it can't be a mail link or an external link, so we can return early

    that means we can also do away with the $validated variable and remove another layer of nesting, and therefore reduce the cyclic complexity

11. +++ b/src/Plugin/Validation/Constraint/LinkyLinkConstraintValidator.php
    @@ -0,0 +1,65 @@
    +      $has_mail_to = $validated = (bool) preg_match('/^mailto:[\w]+/i', $uri);
    

    same comment here re parse_url

12. +++ b/src/Plugin/Validation/Constraint/LinkyLinkConstraintValidator.php
    @@ -0,0 +1,65 @@
    +      if (empty($constraint->settings['email'])) {
    +        if ($has_mail_to) {
    

    and re making this a single if

13. +++ b/src/Plugin/Validation/Constraint/LinkyLinkConstraintValidator.php
    @@ -0,0 +1,65 @@
    +      if (!$url->isExternal()) {
    +        $valid_external = FALSE;
    

    we can return here and avoid the else and the following if(!$valid_external), again simplifying the code.

    In my opinion, using else is always a chance to refactor for an early return

Nice work 😎

This is nearly ready in my book, just some minor cleanup

1. +++ b/config/install/linky.settings.yml
   @@ -0,0 +1,3 @@
   +  email: 1
   +  telephone: 1
   
   +++ b/config/schema/linky.schema.yml
   @@ -15,3 +15,18 @@ field.formatter.settings.linky_label:
   +          type: integer
   ...
   +          type: integer
   
   +++ b/linky.install
   @@ -38,3 +38,15 @@ function linky_update_8101() {
   +  $linky_settings->set('storage.telephone', 1);
   +  $linky_settings->set('storage.email', 1);
   

   let's make these booleans

2. +++ b/config/install/linky.settings.yml
   @@ -0,0 +1,3 @@
   \ No newline at end of file
   

   needs a blank line here

3. +++ b/linky.module
   @@ -87,3 +89,25 @@ function linky_entity_predelete(EntityInterface $entity) {
   +function _linky_get_settings(): ImmutableConfig {
   +  return \Drupal::config('linky.settings');
   

   there's only one use of this, let's inline it

4. +++ b/src/Form/LinkySettingsForm.php
   @@ -0,0 +1,103 @@
   + * Class LinkySettingsForm.
   + *
   + * @package \Drupal\linky\Form
   

   this doesn't meet our coding standards https://www.drupal.org/node/1354

   suggest

   'Provides a settings form for linky'

   and remove the @package

5. +++ b/src/Form/LinkySettingsForm.php
   @@ -0,0 +1,103 @@
   +  private function getConfigName(): string {
   ...
   +  public function getFormId(): string {
   ...
   +  public function buildForm(array $form, FormStateInterface $form_state): array {
   

   love the typehinting

6. +++ b/src/Form/LinkySettingsForm.php
   @@ -0,0 +1,103 @@
   +    $storage_default_values = $this->transformStorageValues($config->get('storage'), TRUE);
   ...
   +    $storage_form_values = $this->transformStorageValues($form_values['storage']);
   

   I don't think we need this helper method - if we switch these to booleans, simply $config->set('storage', array_values(array_filter($form_state->get('storage'))) will cover this

7. +++ b/src/Form/LinkySettingsForm.php
   @@ -0,0 +1,103 @@
   +      '#title' => $this->t('Storage'),
   +      '#description' => $this->t("By checking the options you're allowing them to be stored."),
   

   could we make these

   'Supported additional schemes'

   and

   'Select additional schemes to support in addition to HTTP and HTTPS'

8. +++ b/src/Plugin/Validation/Constraint/LinkyLinkConstraintValidator.php
   @@ -0,0 +1,52 @@
   +    if ((!$item = $value->first()) ||
   ...
   +        $this->context->buildViolation($constraint->notAllowedMessage)->setParameters(['@uri' => $uri])->atPath('0.uri')->addViolation();
   

   note to self: this is a single cardinality field, so this is correct

9. +++ b/tests/src/Kernel/LinkyLinkConstraintTest.php
   @@ -0,0 +1,161 @@
   +class LinkyLinkConstraintTest extends LinkyKernelTestBase {
   

   😍 nice tests

+++ b/config/install/linky.settings.yml
@@ -0,0 +1,3 @@
+additional_schemes:

+++ b/config/schema/linky.schema.yml
@@ -15,3 +15,18 @@ field.formatter.settings.linky_label:
+    storage:

these don't match anymore, other than that we're good to go

Patch LGTM nice work! Just some observations and suggestions.

1. +++ b/linky.module
   @@ -87,3 +89,15 @@ function linky_entity_predelete(EntityInterface $entity) {
   +function linky_entity_base_field_info_alter(array &$fields, EntityTypeInterface $entity_type) {
   

   I think we need this as we can't add constraint in Linky.php as entity level constraints are not supported.

   Maybe add a reason why we have to implement an alter hook.

2. +++ b/src/Plugin/Validation/Constraint/LinkyLinkConstraintValidator.php
   @@ -0,0 +1,52 @@
   +    if ((!$item = $value->first()) ||
   

   Maybe add a comment why first.

3. +++ b/src/Plugin/Validation/Constraint/LinkyLinkConstraintValidator.php
   @@ -0,0 +1,52 @@
   +        $this->context->buildViolation($constraint->notSupportedMessage)->setParameters(['@uri' => $uri])->atPath('0.uri')->addViolation();
   ...
   +        $this->context->buildViolation($constraint->notSupportedMessage)->setParameters(['@uri' => $uri])->atPath('0.uri')->addViolation();
   ...
   +      $this->context->buildViolation($constraint->invalidMessage)->setParameters(['@uri' => $uri])->atPath('0.uri')->addViolation();
   

   I think 0.uri is fine as the cardinality is 1 here.

Fixed on commit

diff --git a/src/Plugin/Validation/Constraint/LinkyLinkConstraintValidator.php b/src/Plugin/Validation/Constraint/LinkyLinkConstraintValidator.php
index 9bb91f9..d07af51 100644
--- a/src/Plugin/Validation/Constraint/LinkyLinkConstraintValidator.php
+++ b/src/Plugin/Validation/Constraint/LinkyLinkConstraintValidator.php
@@ -17,8 +17,8 @@ class LinkyLinkConstraintValidator extends ConstraintValidator {
    * {@inheritDoc}
    */
   public function validate($value, Constraint $constraint) {
-    // This validator is applied to the linky 'link' field of which there
-    // a cardinality of 1, therefore we only need the first item.
+    // This validator is applied to the linky 'link' field which has a
+    // cardinality of 1, therefore we only need the first item.
     /** @var \Drupal\linky\Plugin\Validation\Constraint\LinkyLinkConstraint $constraint */
     if ((!$item = $value->first()) ||
       !$item instanceof LinkItemInterface) {

This will go out as 8.x-1.0-beta1