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!

[argiepiano]

In the dev meeting on 9/19/24 @quicksketch suggested to move all specialized properties within a single, unique property (e.g. #link_field_options). @laryn, in some ways this is something we had discussed and you had started doing.

I have mixed feelings about this: we haven't done this for complex elements like managed_file, options or date, which also add root properties that are unique to those elements and not used anywhere else. I can't find an example of an element that has multi-level properties (well, perhaps #ajax may be an example of a multi-level property, but that's different, as the property #ajax NEEDS specific sub-properties to work.)

It would be pretty straightforward to do this, however. It's just a matter of re-ordering things in the info hook, element form builder, validation and in the widget form builder.

@laryn, if you wish, take on from here. It may take me some time to get back to this.

[quicksketch]

Thanks @argiepiano for summarizing. I had another thought here as well that we might consider.

I think the Form API name #link_field might be left over from ancient CCK (the Drupal 6 predecessor to Field module) days. At that time, every module that provided a CCK Field did so through hook_element_info(), resulting in #types like text_field, email_field, link_field, etc. But since nearly every field did not work in Form API outside of CCK usage, that approach was scrapped when Field module was written for Drupal 7. Some contrib modules (like Link) may have continued to unnecessarily use hook_element_info().

Where I'm going with all this is that either way we go with the Form API properties, we should probably just simplify this element type down to $element['#type'] = 'link'. It was only link_field because it intended to only be used as a Field and not a stand-alone Form API element. If we're changing the purpose, the type should be changed as well. Since this type is not reusable outside of Link module currently, the renaming shouldn't cause much of a problem.

[argiepiano]

Thanks for the comment, @quicksketch. I'm unclear about the extent of the simplification. Are you talking just about the name of the element, or removing most of the properties from the element and leaving them as part of the widget?

I've never had the need to use link_field as a fapi element. @laryn (in the OP) apparently did, so I'd like to get his thoughts about simplifying this (new) fapi link element to its barebones. In the end what this element is, is at least two text input fields (one for the title and another for the link), plus added input fields for link attributes, plus specific validations (e.g. internal vs external, etc).

[quicksketch]

I guess that's the big question @argiepiano. What is the intended purpose of having a stand-alone #type = link field? Where will it be used and what features can work independently of the field configuration?

Are you talking just about the name of the element, or removing most of the properties from the element and leaving them as part of the widget?

Initially, I only meant we should just rename the #type. But yes, there's a bigger question still about what properties should be supported and how they should be named.

[laryn]

Right now, we have a form element named link (under "Structure elements"), used to generate HTML to display a link from a title and href. We also have an element for link_field (under "Input elements"), used to generate a semi-complex field structure for inputting information about a link including the title and the href. Are you proposing to merge these somehow @quicksketch?

Both are currently documented (albiet lightly) and usable by custom code.

• https://docs.backdropcms.org/form_api#link
• https://docs.backdropcms.org/form_api#link_field

The issue I ran into was that although adding a link_field to a custom form works currently, it generates a bunch of warnings as noted in the original post. As this issue expanded from simply corking up those warning messages, it seems like most of the people who've chimed in have assumed that providing some of the link_field specific validation and functionality would be expected/beneficial. I tend to agree and I think the current state of the PR provides an improvement that most people who decide to use this element would expect to be able to find -- although the documentation needs fleshing out, which I'm happy to work on after we settle the details.

I think we need to make a decision as to whether we provide custom field properties at the base level (as the current PR does) or nest them under a new base level property (like "custom_options", which could potentially be a base level property that gets re-used on other fields that have their own custom options).

[quicksketch]

Great point @laryn, I completely forgot that #type = link already creates an <a href> tag. Please disregard my comments about renaming link to link_field then. :slightly_smiling_face:

I think we need to make a decision as to whether we provide custom field properties at the base level (as the current PR does) or nest them under a new base level property

I'm still on the fence about both directions (and making this a generic field at all). None of the options seems entirely clean to me. What's the use-case you're trying to solve here @laryn where defining separate fields would not be sufficient?

[laryn]

As far as my use-case, I have a few different custom forms on a few different projects that I've started using link_field on. I suppose I could rework these usages to use separate fields for title and href, but there are certainly cases (including one of the ones in question) where a link field may need some of the additional complexity (e.g. the field is optional, but if you enter a URL you also need to enter a title). Also I think from a presentation perspective the link_field makes it clearer that these are part of the same functionality, having them side-by-side.

Are you suggesting that link_field is actually not intended to be used by custom forms at all at this point?

[argiepiano]

My perspective on this is that, given that we have a complex Field API field for links, it makes sense to provide similar functionality to developers using form elements. True, a developer could potentially write code to validate things like @laryn is describing, but given that this code already exists for the Field API link_field, why not provide that at the element level? After all this is what has been done with other complex fapi elements like options, which can technically be used at the form element without a Field API field.

After the discussion above, I vote for:

1. Keep the level of complexity this PR provides for the fapi element link_field
2. Move the new properties inside #custom_options as suggested by @laryn, just to keep the documentation more manageable.

I can provide the above point 2 fairly quickly - I'll try to get to this this week.

If on the other hand, someone else wants to explore another route (e.g. making a barebones link_field fapi element, or even getting rid of the fapi element completely - i.e. force developers to use two text fields and custom validations), then please provide an alternative PR.

[argiepiano]

During the 26 Sept 2024 dev meeting, it was agreed that the best solution was to create a single, unique property like #link_field_options, and put all the custom properties in there. I'll modify the PR in this direction.

[argiepiano]

@laryn, @quicksketch, I'm running into an unexpected issue when moving the custom properties into a single #link_field_options. This may also explain why other elements have not done this in the past.

So, link_element_info() provides defaults that are used if the form builder doesn't provide a specific value for a given property. The problem I'm running into is that, if my form builder provides some (but not all) of the sub-properties within #link_field_options, the rest of the sub-properties' defaults are not populated. My guess is that the form process functions use the + array operator, which doesn't do a deep merge of the array - it only adds defaults for missing top-level properties.

I would appreciate your thoughts about this. An alternative is to manually provide defaults (if missing) in link_field_process() process function, but this defeats the purpose of providing defaults in link_element_info().

[argiepiano]

@laryn, I've figured out a pretty clean way to deal with the problem of the sub-array. PR modified.

[argiepiano]

@laryn, any chance you may be able to review this?

[laryn]

I've tested locally and this is working nicely. Thanks @argiepiano!

I don't feel strongly about categorizing it as a bug request, so per the conversation in the dev meeting last week, I'm also changing from a bug report to a feature request.

[quicksketch]

@argiepiano I posted a code review and found several areas that could use some improvement. And I have a few questions, it seems like there might be unused properties being checked? See https://github.com/backdrop/backdrop/pull/4862#pullrequestreview-2421840751

[argiepiano]

@quicksketch I can take a look, but I see you change this to 2.x-future, for what originally was a bug report. Are we not interested in merging this in 1.x? If this is not a priority anymore, I won't spend more time with this. Please clarify.