Problem/Motivation

#2392845: Add a trait to standardize handling of computed item lists will add a ComputedItemListTrait which takes care of all the entry points of a computed field item list. The current implementation of the computed path field has at least one open (and one fixed) bug:

#2905524: The count() method used on PathFieldItemList behaves differently if isEmpty() has been called.
#2908674: When using get() method directly on PathItem the alias is not loaded

Proposed resolution

Use it in PathFieldItemList in order to handle all the possible cases of accessing the computed value.

Remaining tasks

Review.

User interface changes

Nope.

API changes

Nope.

Data model changes

Nope.

CommentFileSizeAuthor
#12 interdiff.txt818 bytesamateescu
#12 2916300-12.patch7.12 KBamateescu
#9 interdiff.txt1.59 KBamateescu
#9 2916300-9.patch7.14 KBamateescu
#9 2916300-9-combined.patch19.92 KBamateescu
#8 interdiff.txt3.01 KBamateescu
#8 2916300-8.patch6.45 KBamateescu
#8 2916300-8-combined.patch16.19 KBamateescu
#4 interdiff.txt1.36 KBamateescu
#4 2916300-4.patch7.06 KBamateescu
#4 2916300-4-combined.patch16.81 KBamateescu
#2 2916300.patch6.37 KBamateescu
#2 2916300-combined.patch16.86 KBamateescu
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amateescu created an issue. See original summary.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Status: Active » Needs review
FileSize
2916300-combined.patch16.86 KB
2916300.patch6.37 KB

Let's try this.

The last submitted patch, 2: 2916300-combined.patch, failed testing. View results

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Assigned: Unassigned » Berdir
FileSize
2916300-4-combined.patch16.81 KB
2916300-4.patch7.06 KB
interdiff.txt1.36 KB

This should fix the failing REST tests. Since the 'computed' logic has been moved to the item list class, the field item class can now behave like a regular field!

However, the path tests will start failing just like in #2905524-12: The count() method used on PathFieldItemList behaves differently if isEmpty() has been called., because this change also fixes the bug reported over there, so it gets into the same problem with the path widget.

I'm really not sure if the fixes over there are correct or not, so I'll leave it to @Berdir to take a look whenever he can.

The last submitted patch, 4: 2916300-4-combined.patch, failed testing. View results

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented 
+++ b/core/modules/path/src/Plugin/Field/FieldType/PathFieldItemList.php
@@ -34,42 +65,4 @@ public function delete() {
-  protected function ensureLoaded() {

So this is not a BC break?

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented

I don't think so, given that the general behavior of the field is not altered in any way.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Assigned: Berdir » Unassigned
FileSize
2916300-8-combined.patch16.19 KB
2916300-8.patch6.45 KB
interdiff.txt3.01 KB

Ok, I figured out how we can proceed here. We can take the 'langcode' property into account in \Drupal\path\Plugin\Field\FieldType\PathItem::isEmpty(), which effectively makes this field "never empty", which is exactly how it behaves in HEAD.

This keeps the count($node_with_no_path->get('path')) == 1 behavior, even though I think it's wrong, but we can punt on #2905524: The count() method used on PathFieldItemList behaves differently if isEmpty() has been called. to fix that.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
FileSize
2916300-9-combined.patch19.92 KB
2916300-9.patch7.14 KB
interdiff.txt1.59 KB

#2909183: Add path alias (PathItem) field PATCH test coverage was committed today and added some ugly code to the PathItem class, so let's reroll this patch on top of #2392845-133: Add a trait to standardize handling of computed item lists and remove that ugly code as well.

Seems to be working fine still based on a quick local testing.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Title: Use ComputedItemListTrait for the path field type » Use ComputedFieldItemListTrait for the path field type

A slight change in the title is needed as well.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

[…] and remove that ugly code as well.

Hurray!

Quoting myself from #2909183-20: Add path alias (PathItem) field PATCH test coverage (you left a comment at -19 pointing to #9 here):

Yep, makes sense — you're making the code introduced in #2846554: Make the PathItem field type actually computed and auto-load stored aliases obsolete and are replacing it with something more thorough. Hence the additional work-arounds added by this issue are no longer necessary. I'm very glad we introduced extra test coverage here that A) help ensure we don't need to fix this again, B) help validate your patch!

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
FileSize
2916300-12.patch7.12 KB
interdiff.txt818 bytes

The parent issue moved the trait back to the typed data level so we also need a small update here.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Priority: Normal » Major
Issue tags: +blocker, +API-First Initiative
Related issues: +#2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior

This blocks #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior, which is major.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Title: Use ComputedFieldItemListTrait for the path field type » [PP-1] Use ComputedFieldItemListTrait for the path field type
Status: Needs review » Postponed

This is blocked on #2392845: Add a trait to standardize handling of computed item lists.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Postponed » Needs review
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Title: [PP-1] Use ComputedFieldItemListTrait for the path field type » Use ComputedFieldItemListTrait for the path field type
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Reviewed & tested by the community

Queued tests for #12, but given that #9 already was green, I think it's okay to already RTBC this.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: 2916300-12.patch, failed testing. View results

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Reviewed & tested by the community

Must've been a random DrupalCI failure, because #12 is definitely green, despite this being marked NW. Back to RTBC.

  • larowlan committed dbfe03c on 8.5.x 
    Issue #2916300 by amateescu: Use ComputedFieldItemListTrait for the path...

  • larowlan committed ccc4d9c on 8.4.x 
    Issue #2916300 by amateescu: Use ComputedFieldItemListTrait for the path...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Fixed

Committed as dbfe03c and pushed to 8.5.x.

Cherry-picked as ccc4d9c and pushed to 8.4.x.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

This unblocked:

  1. #2905524, see #2905524-29: The count() method used on PathFieldItemList behaves differently if isEmpty() has been called. through -31
  2. #2824851, see #2824851-173: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior through -175

Status: Fixed » Closed (fixed)

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

karenann’s picture

karenann CreditAttribution: karenann as a volunteer commented

Upon updating from 8.4.2 to 8.4.4, I found that nodes created via REST were no longer being created with the defined alias pattern.

To elaborate, I have an alias pattern so that nodes created of the type "event" have the alias pattern of "/event/[node:title]".

After the update, nodes created via REST were being created as "/node/nid", the default method.

Reversing this commit to 8.4.x tree causes my nodes created via REST to be created with the correct URL alias pattern.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

@karenan: That's most likely a problem in combination with pathauto module because the autoloading conflicts with the customized flag it adds there.

If you also add pathauto => FALSE inside path then it probably works again.

karenann’s picture

karenann CreditAttribution: karenann as a volunteer commented

Unfortunately, I can't wrap my brain around how to implement the suggestion @berdir has made. I will gladly accept any additional guidance.

In any event, I appreciate the advice! Thanks!

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

Maybe I misunderstood? You want to have the default generated alias and don't explicitly send an alias?

Either way, I'd recommend you open a bug report in pathauto, I'll try to have a look at it there. A test case or so would be extremely useful in helping to debug and fix this and makes it much more likely that I'll find time to look into it.