Make `link_field` more fully available to custom forms

[laryn]

Description of the bug

Link field form elements are not fully available to custom forms. They can be added to a custom form but result in a variety of warnings:

    Warning: Trying to access array offset on value of type null in field_widget_instance() (line 539 of /var/www/html/core/modules/field/field.form.inc).
    Warning: Trying to access array offset on value of type null in link_field_process() (line 853 of /var/www/html/core/modules/link/link.module).
    Warning: Trying to access array offset on value of type null in link_field_process() (line 861 of /var/www/html/core/modules/link/link.module).
    Warning: Trying to access array offset on value of type null in link_field_process() (line 864 of /var/www/html/core/modules/link/link.module).
    Warning: Trying to access array offset on value of type null in link_field_process() (line 867 of /var/www/html/core/modules/link/link.module).
    Warning: Trying to access array offset on value of type null in link_field_process() (line 867 of /var/www/html/core/modules/link/link.module).
    Warning: Trying to access array offset on value of type null in link_field_process() (line 881 of /var/www/html/core/modules/link/link.module).
    Warning: Trying to access array offset on value of type null in link_field_process() (line 883 of /var/www/html/core/modules/link/link.module).
    Warning: Trying to access array offset on value of type null in link_field_process() (line 889 of /var/www/html/core/modules/link/link.module).

Steps To Reproduce

To reproduce the behavior:

1. Create a simple custom form with a link_field element.
2. Submit the form

Actual behavior

The warnings above show up.

Expected behavior

No warnings.

Additional information

@docwilmot has identified two commits in the Drupal 7 version of Link which address this issue. I'll submit a PR that includes these, as well as a variety of other link_field instance settings.

• D7-2718563 by makbul_khan8, jgalletta, idebr, malcomio, joekers, b.lammers, pifagor: Use link field widget in custom form.
• D7-3276037 by xlin1003, DamienMcKenna: PHP 7.4 Notice: Trying to access array offset on value of type null in field_widget_instance() when use in custom form.

[docwilmot]

I think a test would be a good idea to go along with the new functionality?

[laryn]

Yes, probably so. I'll try to work one up.

[laryn]

Okay, I reworked this PR somewhat and added a very simple test. The initial PR just prevented warnings but didn't actually make the widget customizable. What I've done is:

• changed how custom forms implement some of the instance settings, underneath a #instance key in the form array (as seen in the simple test module included in the PR)
• check for the presence of expected keys in the _process hook, and if they aren't there check if it's a custom form implementation and insert values from there.
• simple test that just generates a custom form and makes sure the widget updates display on valid instance settings
• ignored custom form implementations of instance settings that relate to validation, as I think that gets too deep in the weeds for custom forms

[docwilmot]

I left a couple of comments on the PR about the #instance key, which I'd like us to consider. In short, I think this:

 $form['link_test_title_changes'] = array(
    '#type' => 'link_field',
    '#title' => t('Link with title changed'),
    '#tree' => TRUE,
    '#title_maxlength' => 16,
    '#title_label_use_field_label' => TRUE,
    '#show_title' => TRUE,
);

is a better style than:

  $form['link_test_title_changes'] = array(
    '#type' => 'link_field',
    '#title' => t('Link with title changed'),
    '#tree' => TRUE,
    '#instance' => array(
      'settings' => array(
        'title_maxlength' => 16,
        'title_label_use_field_label' => TRUE,
      ),
    ),
  );

ignored custom form implementations of instance settings that relate to validation, as I think that gets too deep in the weeds for custom forms

I think the ability to add #validate_url => TRUE (or #instance['settings']['validate_url'] => TRUE using the current PRs style) should remain with custom fields.

[docwilmot]

And I think we should actually test the element and instance settings work as they should including on form submit.

[laryn]

@docwilmot I'm not necessarily opposed, but one of the reasons I thought it might be cleaner to store all of the instance settings under #instance was because it made it easy to pull the defaults from link_field_info(), and there's a clash with the array key #title otherwise:

• https://github.com/backdrop/backdrop/blob/1.x/core/modules/link/link.module#L36C10-L36C15

But it's not insurmountable.

[argiepiano]

I find the whole approach to the link_field element (in link_field_process()) faulty. Unfortunately this is inherited from D7 and is a bit complex to change. There two levels of form elements:

• Element level (defined by hook_element_info()). These are your vanilla FAPI elements/fields. These elements do not (should not) refer to field API widgets. Therefore the use of field_widget_instance() is wrong at the element process level (link_field_process() should not use it).
• Widget level. Widgets typically use FAPI elements, and therefore build upon their properties. Since widgets ARE related to fields and to instances, therefore the use of field_widget_instance() should be limited to link_field_widget_form()

OK, but given this is what we are dealing with, I suggest two things:

1. Given the patterns used by other elements, I agree with @docwilmot that we shouldn't define a property subarray, and the new properties should be at the root level.
2. I suggest that we add the the new properties' defaults in the element definition in hook_element_info() instead of doing them in the process function.

[laryn]

@argiepiano Sorry, I just saw your comment and I've just submitted changes to the PR. Do you mind glancing it over and seeing what you think?

[argiepiano]

Do you mind glancing it over and seeing what you think?

I can do that - probably later today.

[argiepiano]

@laryn I was about to test and look into this, but this needs rebasing. Will you be able to rebase?

EDIT: nevermind. The patch works fine with the dev verrsion.

[laryn]

@argiepiano I'm rebasing it anyway.

[argiepiano]

Thanks. Working on this at the moment. Your PR looks good, but I'm going to try to refactor things more than you did, to keep the two layers completely separate (Element API and Field API - this is better in your PR, but there are still a few things that are not completely decoupled) and to streamline things like default values etc. This may take me a couple of days. I'll try to submit an alternative PR, and then you can cherrypick from there if you want.

[argiepiano]

@laryn, I've submitted an alternative PR, based on yours. Sorry, I don't know how to do PR to existing PRs.

This refactoring has the goal of providing a (very complex) Link FAPI element that doesn't produce warnings (as in the OP) or errors, and that includes most validations at the element level (instead of the widget level). Validations related to multiple cardinality Field API fields has been kept at the widget level (since element validation is agnostic about cardinality).

This is a summary of what I've done:

• I largely refactored the FAPI element functions link_element_info(), link_field_process() and link_field_element_validate()
  • In a nutshell, I moved most of the validations to the element level, and created several new properties and their defaults. The new element properties are only those that relate to the form element itself. Link provides several instance settings that are related to the display of the Link. Those are not included at the element level since Element API only deals with form elements and their validations, not the display of entered field data.
• I refactored the way the Field API widget relates to the element
• I removed unnecessary helper functions that were there to avoid redundancy of code. Now that most validation is done at the element level, and now that the element provides its own defaults, there is no need for those
• I've preserved your new tests

The functionality and behavior of the widget has not changed at all. The instance structure hasn't changed (so there is no BC breaking).

I decoupled the use of field instance info at the element level, except for one quirky validation that was there before, that required the element to access the instance settings, and also access to #delta needed to avoid issues with #required validations of the last, empty elements in multiple cardinality fields. But I've kept this relationship between widget and element to a minimum.

I've also provided copious documentation of the new element properties and how to use them. This should provide an easy learning curve for those who want to use the FAPI element in custom forms.

Let me know what you think.

EDIT: Oh, I also moved around some related functions that were placed very far from others.

[laryn]

@argiepiano Love it. Once again you provide a master class for the rest of us. I'll close my PR in favour of yours. I've done some testing locally and early code review. I expect to do a little more in depth testing early this week.

This issue/PR really mushroomed into something bigger than I expected/intended at the beginning -- take care of some pesky warnings -- but it's certainly a much better place than where we started and the commits from the D7 side that formed the initial basis of things...

Thank you!

[laryn]

Marking WFM!