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.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

amateescu created an issue. See original summary.

amateescu’s picture

Status: Active » Needs review
FileSize
16.86 KB
6.37 KB

Let's try this.

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

amateescu’s picture

Assigned: Unassigned » Berdir
FileSize
16.81 KB
7.06 KB
1.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

+++ 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

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

amateescu’s picture

Assigned: Berdir » Unassigned
FileSize
16.19 KB
6.45 KB
3.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

#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

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

[…] 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

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

Wim Leers’s picture

Title: Use ComputedFieldItemListTrait for the path field type » [PP-1] Use ComputedFieldItemListTrait for the path field type
Status: Needs review » Postponed
Berdir’s picture

Status: Postponed » Needs review
Berdir’s picture

Title: [PP-1] Use ComputedFieldItemListTrait for the path field type » Use ComputedFieldItemListTrait for the path field type
Wim Leers’s picture

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

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

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.

Status: Fixed » Closed (fixed)

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

karenann’s picture

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

@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

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

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.