statamic / cms

The core Laravel CMS Composer package
https://statamic.com
Other
4.07k stars 532 forks source link

ArrayableLink in Statamic >4.32 causes infinite loop due to array augmentation #9105

Open mkwia opened 11 months ago

mkwia commented 11 months ago

Bug description

After upgrading to Statamic 4.33, some of our sites became broken as a result of the changes introduced in #8911

In a situation where Link A -> Link B -> Link C -> Link A the toAugmentedArray method causes an infinite loop which breaks the site.

When testing locally we were able to resolve this by changing toAugmentedArray to toShallowAugmentedArray on https://github.com/statamic/cms/blob/8a60959d2277fd5e097840c52d10418023da6b73/src/Fieldtypes/Link/ArrayableLink.php#L17

This issue seems similar to #9012

If we verify this fully we will submit a pull request.

How to reproduce

Create a link recursion as described above on versions of statamic > 4.32.

We have not been able to consistently reproduce this on all sites but it seems to affect both regex and runtime parsers.

We will update this issue if we find any additional information on this

Logs

No response

Environment

Environment
Application Name: abcdabcdabcdabcd
Laravel Version: 10.32.1
PHP Version: 8.2.12
Composer Version: 2.6.5
Environment: local
Debug Mode: ENABLED
URL: localhost:8080/
Maintenance Mode: OFF

Cache
Config: NOT CACHED
Events: NOT CACHED
Routes: NOT CACHED
Views: CACHED

Drivers
Broadcasting: null
Cache: redis
Database: mysql
Logs: daily
Mail: ses
Queue: redis
Session: file

Statamic
Addons: 6
Antlers: runtime
Stache Watcher: Enabled
Static Caching: Disabled
Version: 4.33.0 PRO

Statamic Addons
doublethreedigital/runway: 5.5.0
edalzell/forma: 2.1
silentz/akismet: 4.0.1
statamic/eloquent-driver: 2.6.1
withcandour/aardvark-seo: 3.0.0
withcandour/statamic-markdown-table: 1.0.0-beta

Statamic Eloquent Driver
Asset Containers: file
Assets: file
Blueprints: file
Collection Trees: file
Collections: file
Entries: eloquent
Forms: file
Global Sets: file
Global Variables: file
Navigation Trees: file
Navigations: file
Revisions: file
Taxonomies: file
Terms: file

Installation

Fresh statamic/statamic site via CLI

Antlers Parser

None

Additional details

No response

duncanmcclean commented 11 months ago

Hey! šŸ‘‹

If you revert your changes in ArrayableLink.php and try commenting out line 869 of vendor/statamic/cms/src/Entries/Collection.php, are you still experiencing the infinite loop issue?

If you are, then this is the same issue as #9012.

duncanmcclean commented 11 months ago

We've reverted #8928 in the latest release which I believe was the cause of this issue.

afonic commented 10 months ago

@duncanmcclean I have the same issue at exactly 4.32.0 and I found my way here. After upgrading my site times out.

The fix @mkwia suggests works.

I am not sure I understand why this fails, in my case it seems like it's part of a code that uses json_encode in some entries that I use as part of a collection that defines a "mega menu" like part of the website. I am guessing json_encode causes some kind of infinite loop.

Any ideas?

To be honest, I am not sure why this was changed at the first place, a link should be a link if I wanted to get the entry data I would use an Entry relationship field instead. I heavily use links and augmenting each of them to bring the whole entry seems like quite a huge performance hit. Not really blaming anyone (tone is sometimes hard to read in text) just trying to understand how I should move forward.

duncanmcclean commented 10 months ago

Okay, re-opening and we can take a further look. There was a couple other augmentation / infinite loop issues that PR caused so I assumed it would fix this one too but obviously not.

To be honest, I am not sure why this was changed at the first place, a link should be a link if I wanted to get the entry data I would use an Entry relationship field instead. I heavily use links and augmenting each of them to bring the whole entry seems like quite a huge performance hit. Not really blaming anyone (tone is sometimes hard to read in text) just trying to understand how I should move forward.

I think it was added so you can do things like {{ link_field:title }} to get the title of the page you're linking to, whilst not needing a separate Entries field.

There's probably something we can do to avoid the infinite loop.

duncanmcclean commented 10 months ago

I am not sure I understand why this fails, in my case it seems like it's part of a code that uses json_encode in some entries that I use as part of a collection that defines a "mega menu" like part of the website. I am guessing json_encode causes some kind of infinite loop.

@afonic What does your template look like? I can't reproduce the bug using the original reproduction steps.

afonic commented 10 months ago

@duncanmcclean it's something like this:

{{ collection:menu as="menu" }}
<component
    :menu='{{ menu | to_json | entities }}'
    :site='{{ site:handle | to_json }}'
>
</component>
{{ /collection:menu }}

I am trying to filter out items until I find what causes it, in the menu collection I heavily use link fields to entries.

duncanmcclean commented 10 months ago

Just to confirm, it isn't fixed if you try upgrading to the latest version?

afonic commented 10 months ago

Nope, it is fixed after reverting back to v4.31 or just reverting the changes made to Link.php

I will investigate further during the weekend.

afonic commented 8 months ago

I managed to do some tests for this, it seems to happen only in nested Link fields, for example I have a Replicator field which has a set that has an Entries field and these entries have a Link field that points to another entry.

However I failed to reproduce it in a clean install so I just changed my blueprints around to move past this.

n-va commented 7 months ago

We're able to recreate this with links when they're referring to each other.

Ex: Page A links to Page B, and Page B links to Page A, they are also nested inside sections.

The fix mkwia suggested with swapping to a ->shallowAugementedArray() in Link\ArrayableLink

Page A

---
id: 633fd516-fa95-401e-824b-48ff972293bb
blueprint: page
title: 'Page A'
sections:
  -
    id: lv0cygaq
    header: test
    buttons:
      -
        id: lv0cyhlk
        text: asdad
        link: 'entry::ee0d09b8-b33f-4541-9d9f-08ece596f2d8'
    invert_colour: false
    include_icon: false
    type: call_to_action
    enabled: true
updated_by: 6
updated_at: 1713149938
parent: cd3cda89-07b6-4ce0-a79d-cfbab52d2931
---

Page B

---
id: ee0d09b8-b33f-4541-9d9f-08ece596f2d8
blueprint: page
title: 'Page B'
sections:
  -
    id: lv0cxm72
    header: asda
    buttons:
      -
        id: lv0cxn1k
        text: ee
        link: 'entry::633fd516-fa95-401e-824b-48ff972293bb'
    invert_colour: false
    include_icon: false
    type: call_to_action
    enabled: true
parent: cd3cda89-07b6-4ce0-a79d-cfbab52d2931
updated_by: 6
updated_at: 1713149951
---
github-actions[bot] commented 5 months ago

This issue has not had recent activity and has been marked as stale ā€” by me, a robot. Simply reply to keep it open and send me away. If you do nothing, I will close it in a week. I have no feelings, so whatever you do is fine by me.

jelleroorda commented 2 months ago

@duncanmcclean Not 100% sure if it's related; but throwing it here in case you guys can dig up something.

After replacing the toAugmentedArray() with toShallowAugmentedArray() the page loads (but very slowly).

As far as I can tell, we can specify fields (performance tip no. 2) that you want to (bulk) augment for the collection. However, once you cast the collection/pages to JSON, it triggers the Pages ->toJson() method here, which causes a full augmentation of the page (without any selected columns).

Adding $page->selectedQueryColumns(array_keys($data)); to the Structure tag @ line 131 massively speeds up the page, as well as only loads all augmented data that I've specified in my select. This logic should probably (?) be placed somewhere else though, I was just experimenting.