diff --git a/src/Fields/Fields.php b/src/Fields/Fields.php index f6707b50866..e1531f92b89 100644 --- a/src/Fields/Fields.php +++ b/src/Fields/Fields.php @@ -118,10 +118,15 @@ public function only(...$keys): self public function newInstance() { - return (new static) + // Assign the items directly — setItems() would re-resolve every field, then + // setFields() would immediately discard that work. Clone so the two instances + // don't share one collection, which is what setItems() would have given us. + $instance = new static; + $instance->items = clone $this->items; + + return $instance ->setParent($this->parent) ->setParentField($this->parentField, $this->parentIndex) - ->setItems($this->items) ->setFields($this->fields) ->setFilled($this->filled); } diff --git a/src/Fieldtypes/Entries.php b/src/Fieldtypes/Entries.php index 402540e0a9d..8b070789cac 100644 --- a/src/Fieldtypes/Entries.php +++ b/src/Fieldtypes/Entries.php @@ -379,18 +379,23 @@ protected function getCreatables() protected function authorizeItemData($id): bool { - return $this->authorizeViewable(Entry::find($id)); + return $this->authorizeViewable($this->findEntry($id)); } protected function toItemArray($id) { - if (! $entry = Entry::find($id)) { + if (! $entry = $this->findEntry($id)) { return $this->invalidItemArray($id); } return (new EntryResource($entry, $this))->resolve()['data']; } + protected function findEntry($id) + { + return $this->itemCache[$id] ??= Entry::find($id); + } + protected function collect($value) { return new \Statamic\Entries\EntryCollection($value); diff --git a/src/Fieldtypes/Relationship.php b/src/Fieldtypes/Relationship.php index d7cf287998c..16d93057144 100644 --- a/src/Fieldtypes/Relationship.php +++ b/src/Fieldtypes/Relationship.php @@ -31,6 +31,14 @@ abstract class Relationship extends Fieldtype '_' => '_', // forces an object in js ]; protected $formStackSize; + protected array $itemCache = []; + + public function __clone() + { + // The fieldtype repository hands out clones of a single instance per handle, + // so without this the cache would be inherited by unrelated fields. + $this->itemCache = []; + } protected function configFieldItems(): array { diff --git a/src/Fieldtypes/Terms.php b/src/Fieldtypes/Terms.php index 1501d4c29c2..7e608fd8ca0 100644 --- a/src/Fieldtypes/Terms.php +++ b/src/Fieldtypes/Terms.php @@ -418,20 +418,14 @@ protected function getCreatables() protected function authorizeItemData($id): bool { - if ($this->usingSingleTaxonomy() && ! Str::contains($id, '::')) { - $id = "{$this->taxonomies()[0]}::{$id}"; - } - - return $this->authorizeViewable(Term::find($id)); + return $this->authorizeViewable($this->findTerm($id)); } protected function toItemArray($id) { - if ($this->usingSingleTaxonomy() && ! Str::contains($id, '::')) { - $id = "{$this->taxonomies()[0]}::{$id}"; - } + $id = $this->normalizeTermId($id); - if (! $term = Term::find($id)) { + if (! $term = $this->findTerm($id)) { return $this->invalidItemArray($id); } @@ -457,6 +451,22 @@ protected function toItemArray($id) ]; } + protected function normalizeTermId($id): string + { + if ($this->usingSingleTaxonomy() && ! Str::contains($id, '::')) { + return "{$this->taxonomies()[0]}::{$id}"; + } + + return $id; + } + + protected function findTerm($id) + { + $id = $this->normalizeTermId($id); + + return $this->itemCache[$id] ??= Term::find($id); + } + protected function getColumns() { $columns = [Column::make('title')]; diff --git a/src/Fieldtypes/Users.php b/src/Fieldtypes/Users.php index 298771dc765..35597809570 100644 --- a/src/Fieldtypes/Users.php +++ b/src/Fieldtypes/Users.php @@ -104,12 +104,12 @@ public function preProcess($data) protected function authorizeItemData($id): bool { - return $this->authorizeViewable(User::find($id)); + return $this->authorizeViewable($this->findUser($id)); } protected function toItemArray($id, $site = null) { - if ($user = User::find($id)) { + if ($user = $this->findUser($id)) { $canViewUsers = $this->canViewUser($user); return [ @@ -123,6 +123,11 @@ protected function toItemArray($id, $site = null) return $this->invalidItemArray($id); } + protected function findUser($id) + { + return $this->itemCache[$id] ??= User::find($id); + } + public function getIndexItems($request) { // Don't reveal existence to a user who can't view the listing; return an empty diff --git a/src/Http/Controllers/CP/Fieldtypes/RelationshipFieldtypeController.php b/src/Http/Controllers/CP/Fieldtypes/RelationshipFieldtypeController.php index 899feef822b..a138e037fee 100644 --- a/src/Http/Controllers/CP/Fieldtypes/RelationshipFieldtypeController.php +++ b/src/Http/Controllers/CP/Fieldtypes/RelationshipFieldtypeController.php @@ -56,12 +56,15 @@ private function getConfig($request) // The json may include unicode characters, so we'll try to convert it to UTF-8. // See https://github.com/statamic/cms/issues/566 - $utf8 = mb_convert_encoding($json, 'UTF-8', mb_list_encodings()); - - // In PHP 8.1 there's a bug where encoding will return null. It's fixed in 8.1.2. - // In this case, we'll fall back to the original JSON, but without the encoding. - // Issue #566 may still occur, but it's better than failing completely. - $json = empty($utf8) ? $json : $utf8; + // Fast path: skip encoding detection when already valid UTF-8. + if (! mb_check_encoding($json, 'UTF-8')) { + $utf8 = mb_convert_encoding($json, 'UTF-8', mb_list_encodings()); + + // In PHP 8.1 there's a bug where encoding will return null. It's fixed in 8.1.2. + // In this case, we'll fall back to the original JSON, but without the encoding. + // Issue #566 may still occur, but it's better than failing completely. + $json = empty($utf8) ? $json : $utf8; + } return json_decode($json, true); } diff --git a/tests/Fields/FieldsTest.php b/tests/Fields/FieldsTest.php index 1d7f9532d77..711b27acb3e 100644 --- a/tests/Fields/FieldsTest.php +++ b/tests/Fields/FieldsTest.php @@ -371,6 +371,63 @@ public function it_gets_only_specific_fields() $this->assertEquals(['one', 'two'], $fields->only(['one', 'two'])->all()->keys()->all()); } + #[Test] + public function it_carries_items_over_to_a_new_instance_without_resolving_them_again() + { + $items = [ + ['handle' => 'one', 'field' => ['display' => 'First']], + ['handle' => 'two', 'field' => ['display' => 'Second']], + ]; + + $fields = new Fields($items); + $instance = $fields->newInstance(); + + $this->assertNotSame($fields, $instance); + $this->assertEquals($items, $instance->items()->all()); + + // The already resolved fields get carried over as-is, rather than being + // resolved a second time only for setFields() to throw them away. + $this->assertSame($fields->get('one'), $instance->get('one')); + $this->assertSame($fields->get('two'), $instance->get('two')); + } + + #[Test] + public function replacing_the_items_on_a_new_instance_doesnt_affect_the_original() + { + $fields = new Fields([ + ['handle' => 'one', 'field' => ['display' => 'First']], + ]); + + $instance = $fields->newInstance(); + + $instance->setItems([ + ['handle' => 'two', 'field' => ['display' => 'Second']], + ]); + + $this->assertEquals(['one'], $fields->items()->pluck('handle')->all()); + $this->assertEquals(['two'], $instance->items()->pluck('handle')->all()); + $this->assertTrue($fields->has('one')); + $this->assertFalse($fields->has('two')); + } + + #[Test] + public function mutating_the_items_on_a_new_instance_doesnt_affect_the_original() + { + $fields = new Fields([ + ['handle' => 'one', 'field' => ['display' => 'First']], + ]); + + $instance = $fields->newInstance(); + + $this->assertNotSame($fields->items(), $instance->items()); + + $instance->items()->push(['handle' => 'two', 'field' => ['display' => 'Second']]); + $instance->items()->put(0, ['handle' => 'clobbered', 'field' => ['display' => 'Clobbered']]); + + $this->assertEquals(['one'], $fields->items()->pluck('handle')->all()); + $this->assertEquals(['clobbered', 'two'], $instance->items()->pluck('handle')->all()); + } + #[Test] public function converts_to_array_suitable_for_rendering_fields_in_publish_component() { diff --git a/tests/Fieldtypes/EntriesTest.php b/tests/Fieldtypes/EntriesTest.php index 7416ca21d04..752d5bcab2d 100644 --- a/tests/Fieldtypes/EntriesTest.php +++ b/tests/Fieldtypes/EntriesTest.php @@ -376,6 +376,24 @@ public function it_doesnt_localize_when_select_across_sites_setting_is_enabled() $this->assertEquals(['one', 'two', 'three', 'four'], $augmented->get()->map->slug()->all()); } + #[Test] + public function it_doesnt_inherit_the_item_cache_from_another_fieldtype_instance() + { + $this->actingAs(Facades\User::make()->makeSuper()); + + $field = new Field('test', ['type' => 'entries', 'collections' => ['blog']]); + + // The fieldtype repository hands out clones of a single instance per handle, + // so one field's lookups must not leak into another's. + $first = $field->fieldtype(); + $this->assertFalse($first->getItemData(['123'])->first()['invalid'] ?? false); + + Facades\Entry::find('123')->delete(); + + $second = $field->fieldtype(); + $this->assertTrue($second->getItemData(['123'])->first()['invalid']); + } + public function fieldtype($config = [], $parent = null) { $field = new Field('test', array_merge([ diff --git a/tests/Http/Controllers/CP/Fieldtypes/RelationshipFieldtypeControllerTest.php b/tests/Http/Controllers/CP/Fieldtypes/RelationshipFieldtypeControllerTest.php new file mode 100644 index 00000000000..178050352ca --- /dev/null +++ b/tests/Http/Controllers/CP/Fieldtypes/RelationshipFieldtypeControllerTest.php @@ -0,0 +1,74 @@ +save(); + EntryFactory::id('123')->collection('blog')->slug('one')->data(['title' => 'One'])->create(); + } + + private function request($config, $selections = ['123']) + { + return $this + ->actingAs(Facades\User::make()->makeSuper()) + ->post(cp_route('relationship.data'), [ + // JSON.stringify() in the browser leaves multibyte characters as-is, + // so don't let PHP escape them to \uXXXX sequences here. + 'config' => base64_encode(json_encode($config, JSON_UNESCAPED_UNICODE)), + 'selections' => $selections, + ]); + } + + #[Test] + public function it_gets_item_data_for_the_configured_fieldtype() + { + $this->request(['type' => 'entries', 'collections' => ['blog']]) + ->assertOk() + ->assertJsonPath('data.0.title', 'One'); + } + + #[Test] + public function it_doesnt_mangle_multibyte_characters_in_the_config() + { + // The base64-encoded JSON config used to be run through mb_convert_encoding() + // with a detection list, which could misdetect perfectly valid UTF-8 as another + // encoding and corrupt it. Emoji were mangled into Cyrillic. See #566. + (new class extends Relationship + { + public static function handle() + { + return 'config_echo'; + } + + protected function toItemArray($id) + { + return ['id' => $id, 'title' => $this->config('display')]; + } + + public function getIndexItems($request) + { + return collect(); + } + })::register(); + + $display = '😀👍'; + + $this->request(['type' => 'config_echo', 'display' => $display]) + ->assertOk() + ->assertJsonPath('data.0.title', $display); + } +}