Adjust dump method in Extended\ACF\Fields\Field to utilize object cloning

[marcoluzi]

This PR will fix #153.

Description

This pull request refactors the get method in the Extended\ACF\Fields\Field class to utilize PHP object cloning. The changes ensure that field instances, particularly those with sub_fields, layouts, and conditional_logic, are processed independently, preventing unintended modifications to the original objects during method execution.

Changes Made

• Object Cloning: Implemented object cloning in the get method to create independent instances of sub_fields, layouts, and conditional_logic before processing.
• Safety Checks: Added conditional checks to ensure clone is only applied to objects, safeguarding against potential errors when dealing with non-object types.

[vinkla]

Thanks for the pull request. This approach may not work as we create field keys on the fly. If the keys are generated without a parent, they could become invalid. Is there a way to clone the field within the dump method instead?

[marcoluzi]

That would only be symptom control. I actually stumbeled across this bug when I was trying to use the get method itself. So if we would adjust the dump() method, the problem would still occure if someone would use the get() method.

Here is a code snippet of how I want to utilize the get() method:

private function registerOptionPageContext(array $option_page): void
{
	if (!isset($option_page['page_settings']['timber_context_key'])) {
		return;
	}

	add_filter('timber/context', function ($context) use ($option_page) {
		$context[$option_page['page_settings']['timber_context_key']] = $this->getFieldsContext($option_page);
		return $context;
	});
}
private function getFieldsContext(array $option_page): array
{
	$fieldsContext = [];
	$fields = $option_page['acf_settings']['fields'] ?? [];

	foreach ($fields as $field) {
		$key = $field->get()['key'];
		$name = $field->get()['name'];
		$fieldsContext[$name] = get_field($key, 'option');
	}

	return $fieldsContext;
}

With those two methods I am looping through all the fields registered in option pages and and adding them to the timber contect, to make them available in the frontend. I am using the get() method to get the field key, so i can use the get_field() function. This all needs to work dynamically since extended-acf generates the keys on the fly.

[vinkla]

The get method is accessible but marked as @internal. It requires a parent key to function properly. The use case you provided goes beyond the scope of the project.

With those two methods I am looping through all the fields registered in option pages and and adding them to the timber context.

I'm curious. Why aren't the fields available by default in Timber?

[marcoluzi]

That's a very good question. I've never questioned this, since it has always been like this in the theme since I work here. I checked the Timber Docs and they recommend to use get_fields('options'); to get all values registered on option pages. This works for me. Thanks for the hint. Using this, my usecase of extended-acf is working now.

As for this PR: I think it would still be better to find a solution for fields that utilize child fields. As of now, even if you don't use the get() method, as you suggested, the dump() method is broken for fields with subfields. So I would either prevent the usage of dump() on those fields or maybe trying to find a solution for this.

I'm not that familiar with the codebase of this repo or the acf source code. But could something like this be a solution?

/** @internal */
public function get(string|null $parentKey = null): array
{
    // Resolve the parent key to ensure it's valid within the current context
    $resolvedParentKey = $parentKey !== null ? Key::resolveParentKey($parentKey, $this->settings['name']) : null;

    $key =
        $this->settings['key'] ??
        ($resolvedParentKey !== null ? $resolvedParentKey . '_' : '') . Key::sanitize($this->settings['name']);

    if ($this->type !== null) {
        $this->settings['type'] = $this->type;
    }

    if (isset($this->settings['conditional_logic'])) {
        $this->settings['conditional_logic'] = array_map(
            fn($rules) => is_object($rules) && method_exists($rules, 'get')
                ? $rules->get($resolvedParentKey)
                : $rules,
            $this->settings['conditional_logic'],
        );
    }

    if (isset($this->settings['layouts'])) {
        $this->settings['layouts'] = array_map(
            fn($layout) => is_object($layout) && method_exists($layout, 'get')
                ? $layout->get($key)
                : $layout,
            $this->settings['layouts'],
        );
    }

    if (isset($this->settings['sub_fields'])) {
        $this->settings['sub_fields'] = array_map(
            fn($field) => is_object($field) && method_exists($field, 'get')
                ? $field->get($key)
                : $field,
            $this->settings['sub_fields'],
        );
    }

    if (isset($this->settings['collapsed'], $this->settings['sub_fields'])) {
        foreach ($this->settings['sub_fields'] as $field) {
            if (is_array($field) && isset($field['name']) && $field['name'] === $this->settings['collapsed']) {
                $this->settings['collapsed'] = $field['key'] ?? $this->settings['collapsed'];
                break;
            }
        }
    }

    $this->settings['key'] ??= Key::generate($key, $this->keyPrefix);

    return $this->settings;
}

[vinkla]

I may be mistaken, but this could also pose a problem since we require the field group key to generate the initial parent key. Is there a way to resolve this using only the dump method? If not, we could probably just dump the settings array instead in the dump method.

[marcoluzi]

I think cloning the object before getting the settings is the most straightforward solution. Returning the Field after calling get() on it seems to break it, not the get() itself. So let's just call get() on a clone.

[vinkla]

Thank you for the update. Have you tried this approach? The error still seems to be occurring on my end.

[marcoluzi]

@vinkla It does work on my end. 🤔

[vinkla]

I know why I still see the error on my side. It's because I have nested repeaters. We might need to map over the fields first and clone them if they have subfields.

[marcoluzi]

@vinkla The fields should now be cloned recursively.

[marcoluzi]

Do you need me to do anything else?

[vinkla]

Sorry for the delay, haven't had time to look into this. Will look at it in the future.