spicywebau / craft-embedded-assets

Manage YouTube videos, Instagram photos, Twitter posts and more as first class assets in Craft CMS
MIT License
171 stars 34 forks source link

TypeError: Cannot read properties of undefined (reading 'folderId') #240

Closed arifje closed 10 months ago

arifje commented 10 months ago

Bug Description

Since our update to Craft 4.5.1 and Embedded Assets 3.1.6, we can't save the embeds anymore.

main.ts:67 Uncaught TypeError: Cannot read properties of undefined (reading 'folderId')
    at o.c [as _currentGetActionTarget] (main.ts:67:32)
    at o._getActionTarget (EmbeddedAssets.ts:76:17)
    at h._getActionTarget (EmbeddedAssets.ts:17:39)
    at h.save (Form.ts:137:72)
    at HTMLDivElement.<anonymous> (Modal.ts:57:45)
    at HTMLDivElement.dispatch (jquery.js?v=1692888808:2:43392)
    at v.handle (jquery.js?v=1692888808:2:41387)

Steps to reproduce

Expected behaviour

No response

Embedded Assets version

3.1.6

Craft CMS version

4.5.1

joshuabaker commented 10 months ago

Also seeing this with Embedded Assets 3.1.6 and Craft 4.5.3. Clients are unable to update content as a result.

The error originates from the following line:

https://github.com/spicywebau/craft-embedded-assets/blob/b1d1fbac53b6e31718e95f709288c2e84dc90870/src/assets/main/src/scripts/main.ts#L67

It doesn’t look like sourcePath is present on the asset select input.

joshuabaker commented 10 months ago

Could be just that we need the extra check or optional chain as referencing properties of a non-object will throw.

Here’s a possible drop-in replacement for the getActionTarget method.

  const getActionTarget: () => Object = () => {
    if (Array.isArray(this.sourcePath)) {
      // Craft 4.4 subfolder compatibility
      const currentFolder = this.sourcePath[this.sourcePath.length - 1]

      if (currentFolder?.folderId) {
        return {
          targetType: 'folder',
          targetId: currentFolder.folderId
        }
      }
    }

    const [targetType, targetUid] = this.sourceKey.split(':')

    if (targetType && targetUid) {
      return { targetType, targetUid }
    }

    return {}
  }

I’ve also stripped the typeof checks in favour of clearer and type-checked conditions. It’s absolutely a personal preference, but I think the conditions and destructuring assignment syntax make it a bit more readable.

All of this is assuming that currentFolder should ever be nullish.

ttempleton commented 10 months ago

This is working as expected for me, e.g. the following screenshot shows me logging the sourcePath property:

Screenshot 2023-08-30 at 20 25 54

Not really willing to make changes without being able to reproduce the error, since I don't want to make a change that breaks something else, and sourcePath is actually a Craft.AssetIndex property and I'm unsure what case causes it to be empty. Any extra information that could help identify the cause (e.g. a specific field configuration if this only happens when editing entries) is appreciated.

joshuabaker commented 10 months ago

Thanks, @ttempleton. Where are you seeing that array you’re referencing?

joshuabaker commented 10 months ago
Field settings field settings
arifje commented 10 months ago

Our field config;

Screenshot 2023-08-30 at 13 16 07

ttempleton commented 10 months ago

Thanks both - I can reproduce the error and it is related to the use of variables when restricting assets to a single location.

Just want to confirm that this has only started happening when upgrading to Craft 4.5? This is just for the purpose of updating the changelog.

joshuabaker commented 10 months ago

Thanks, @ttempleton. Glad we were able to help diagnose this.

I was seeing this with earlier versions (e.g. Craft 4.4.17).

ttempleton commented 10 months ago

Thanks - just released 3.1.7 with the fix.

joshuabaker commented 10 months ago

Amazing! Thanks, @ttempleton. Can confirm this is up and running again for us. 🙏🏻