Problem/Motivation

A LinkItem field is empty if its value is an empty string or NULL. If a NULL value is passed to any of the Link field's constraint validator classes, then the following deprecation warning is produced when they call LinkItemInterface::getUrl().

Deprecated function: preg_match(): Passing null to parameter #2 ($subject) of type string is deprecated in Drupal\Core\Url::fromUri() (line 281 of core/lib/Drupal/Core/Url.php).

Steps to reproduce

See steps in https://www.drupal.org/project/drupal/issues/3348390#comment-15228424 as long as that patch is not applied/committed.

  • Install the workspaces module
  • Add a link field to the page content type, cardinality unlimited, standards for other settings
  • Create a content of type page, adding a link with values and clicking the "Add another item" button not populating the 2nd link item
  • Save the node and re-open the edit form

OR

  • Go to admin/config/development/logging, select 'All' and save.
  • Add a link field with Unlimited cardinality to article content type.
  • Create a new article, add one link and save
  • Add a form after to article edit form
  • Load the node and call validate() method.[$entity = $form_state->getFormObject()->getEntity(); $entity->validate();]
  • Now try to edit the previously created node.

https://www.drupal.org/files/issues/2024-01-15/3387013-error.png

Proposed resolution

The constraint validators are heavily dependent on the passed $value being a LinkItemInterface object. Check that the value is an instance of the interface and that it is not empty before validating. Remove the existing if (isset($value)) {...} conditions because they're basically pointless since an object will always pass that check.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#13 3387013-nr-bot.txt1.31 KBneeds-review-queue-bot
#9 3387013-error.png379.05 KBakhil babu
#2 empty_checks_link_validators-3387013-2.patch3.88 KBs_leu

Issue fork drupal-3387013

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

s_leu created an issue. See original summary.

s_leu’s picture

s_leu commented
Status: Active » Needs review
StatusFileSize
new empty_checks_link_validators-3387013-2.patch3.88 KB

Here a basic patch to illustrate the idea of how to change this. The added code should probably be moved into a base class if this approach makes sense at all.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Thank you for reporting.

Wonder if we could get a test-only patch showing the issue.

Akhil Babu made their first commit to this issue’s fork.

Akhil Babu opened merge request !6162

Akhil Babu changed the visibility of the branch 3387013-check-of-value-11 to hidden.

Akhil Babu opened merge request !6163

Akhil Babu changed the visibility of the branch 3387013-check-of-value to hidden.

akhil babu’s picture

akhil babu
he/him
commented
Issue summary: View changes
StatusFileSize
new 3387013-error.png379.05 KB
akhil babu’s picture

akhil babu
he/him
commented
Issue summary: View changes
akhil babu’s picture

akhil babu
he/him
commented

I was able to recreate this issue, but only with the help of custom code.

  • Go to admin/config/development/logging, select 'All' and save.
  • Add a link field with Unlimited cardinality to article content type.
  • Create a new article, add one link and save
  • Add a form after to article edit form
  • Load the node and call validate() method.[$entity = $form_state->getFormObject()->getEntity(); $entity->validate();]
  • Now try to edit the previously created node.

The errors appear only when validate() is called on node object when we are in the node edit form. For example

/**
 * Implements hook_preprocess_HOOK() for page.html.twig.
 */
function MY_MODULE_preprocess_page($variables) {
  \Drupal::entityTypeManager()->getStorage('node')->load(1)->validate();
}

will not trigger any errors when the node is viewd. But will trigger error when edit form is loaded.

When the edit form is loaded after adding a link item to an unlimited cardinality link field, Drupal automatically appends a new link widget below the existing one to add a second link item. Somehow, this second item is also validated when the node is validated from the edit form, and this triggers the NULL warning. This won't happen when the node is loaded from any place other than the node edit form. So, maybe the logic in the validate() method needs to be fixed.

I have created a test to show the bug.

akhil babu’s picture

akhil babu
he/him
commented
Status: Needs work » Needs review
needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3387013-nr-bot.txt1.31 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

dcam changed the visibility of the branch 3387013- to hidden.

dcam changed the visibility of the branch 11.x to hidden.

dcam opened merge request !13130

dcam’s picture

dcam commented
Title: Check of $value not covering LinkItem cases in link contraint validators » Check LinkItemInterface::isEmpty() before validating
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs tests

I converted the patch in #2 to an MR and added Unit tests.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Bug Smash Initiative

Ran test-only change here https://git.drupalcode.org/issue/drupal-3387013/-/jobs/6473897 and got 4 failures that line up with the 4 new tests, create to make a unit test!

Refactoring of the constraints to check instance of LinkItemInterface makes sense.

No feedback.

dcam’s picture

dcam commented
Status: Reviewed & tested by the community » Needs review

I wasn't happy with the validators returning if the input wasn't a LinkItemInterface. So I looked up the Symfony validator docs. They show an UnexpectedValueException being thrown in instances just like what we're dealing with. So I'm taking that as a best practice.

I played around with the idea of putting this early validation in a base class, but it wasn't working out cleanly. So I scrapped the idea.

dcam’s picture

dcam commented
Status: Needs review » Postponed

Since I made changes I've decided to postpone this on #3077149: Duplicate inline form errors in external Link widgets instead of the other way around like we've been doing. The other issue has a more complete LinkTypeConstraintValidatorTest test class. I'd also consider the other issue to be the more serious bug since it can happen simply by enabling Inline Form Errors whereas this one seems like an edge case potentially requiring custom code.

dcam’s picture

dcam commented
Status: Postponed » Needs work

This is unblocked and needs a rebase to integrate the new test that was added by both this issue and the blocker.

dcam’s picture

dcam commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

This may need a manual rebase. Least gitlab is throwing a fit.

dcam’s picture

dcam commented
Status: Needs work » Needs review

I have no idea what was going on with it. The "Update fork" button worked on the branch page. It's reporting as being mergeable now.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Feedback for this one appears to be addressed.

Tested following the steps and able to reproduce, test-only has already been ran before the rebase.
Fixes do address the problem and seems to be copied around (which is fine). May be cool eventually to have some trait that can be shared but out of scope of this issue.

LGTM

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

Needs a rebase, likely due to #3093118: Move link title validation from LinkWidget into a constraint

dcam’s picture

dcam commented
Status: Needs work » Reviewed & tested by the community

The "Update Fork" button on the branch page worked to rebase it.

catch’s picture

catch
he/him
Primary language English
commented

It looks like the new validator in that issue might also need an isEmpty() check?

dcam’s picture

dcam commented

It looks like the new validator in that issue might also need an isEmpty() check?

I don't think so. This fixes bugs where getUrl() is called without first checking if there is a URL. The other issue's new validator doesn't call getUrl() because it's about checking the title subfield.

  • catch committed b9564e01 on 11.x 
    fix: #3387013 Check LinkItemInterface::isEmpty() before validating

By:...

  • catch committed a19cb005 on 11.3.x 
    fix: #3387013 Check LinkItemInterface::isEmpty() before validating

By:...
catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 11.3.x-dev
Status: Reviewed & tested by the community » Fixed

Oh that makes sense, couldn't see an actual issue in the latest commit but was thinking about short-circuiting other logic, not loads to short-circuit though.

Committed/pushed to 11.x and cherry-picked to 11.3.x, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

catch closed merge request !13130

Status: Fixed » Closed (fixed)

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