Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return a Fluent instance for block config #46

Merged
merged 1 commit into from
Feb 7, 2025

Conversation

damsfx
Copy link
Contributor

@damsfx damsfx commented Feb 7, 2025

fix #23

Improve consistency between configuration data for blocks whose inspector has been opened or not.

The Illuminate\Support\Fluent class is a useful data type. It allows for the construction of a data "container" similar to an array or instance of stdClass. However, the Fluent class makes it easier to make assumptions about the data the class instance contains.

with arrays

use Illuminate\Support\Fluent;

$data = [
     'a' => 1,
     'b' => 2,
];

dd($data['c']);      // Undefined array key "c"

$fluent = new Fluent($data);

dd($fluent['a']);      // 1  - array like attribute access
dd($fluent->b);       // 2 - object like property access 
dd($fluent['c']);     // null
dd($fluent->c);       // null

with objects

use Illuminate\Support\Fluent;

$data = (object) [
      'a' => 1,
      'b' => 2,
];

dd($data['a']);        // Cannot use object of type stdClass as array

$fluent = new Fluent($data);

dd($fluent['a']);      // 1  - array like attribute access
dd($fluent->b);       // 2 - object like property access 
dd($fluent['c']);     // null
dd($fluent->c);       // null

⚠ If a key is not present, a null value will be return (default behavior).

@LukeTowers LukeTowers merged commit 1db24de into wintercms:main Feb 7, 2025
6 checks passed
@mjauvin
Copy link
Member

mjauvin commented Feb 8, 2025

@damsfx @LukeTowers I am getting the following error after this PR has been merged:
image

If I revert all is good.

Odd thing to note, the error points to the "section" block in my theme even though it is actually defined in my plugin.

@mjauvin
Copy link
Member

mjauvin commented Feb 8, 2025

The problem is here in renderPartial, array_merge() is expecting an array, not a Fluent object:

https://github.com/wintercms/winter/blob/develop/modules/cms/classes/Controller.php#L1070

Update: actually, it's coming from further down the callstack in Twig\Template::display() which tries to yield the values from the array, eventually hit the Fluent object but expecting an array.

@damsfx
Copy link
Contributor Author

damsfx commented Feb 8, 2025

@mjauvin I was also able to reproduce this bug ...

If my page contains a “two-column” block.
The error occurs on {{ renderBlocks(left | default([]))) }} to render a column.

But also, as you, within my section block too ({{ renderBlocks(data.section.blocks) }}).

I have made a dump($this->data) in my section.block file but their is no differences in the data between the old config and the fluent config data.

I've forced the block config to be an array with: $partialData['config'] = json_decode(json_encode($config), true);.
This way block config is always of type array, whether or not the inspector has been opened when editing the page content.

It may be a better solution than the Fluent type in the end.

I'm really sorry for suggesting this non-answer solution.
I apologize.

@mjauvin
Copy link
Member

mjauvin commented Feb 8, 2025

Yeah, I think it's better to force array in json_decode.

@damsfx
Copy link
Contributor Author

damsfx commented Feb 11, 2025

@mjauvin @LukeTowers What's the best way to revert this commit to publish another one?

@LukeTowers
Copy link
Member

@damsfx just make another PR with the reversion / desired proper fix in one

@damsfx
Copy link
Contributor Author

damsfx commented Feb 11, 2025

@mjauvin @LukeTowers Done in #47

@damsfx damsfx deleted the fluent-config branch February 12, 2025 11:49
LukeTowers pushed a commit that referenced this pull request Feb 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Block config not always of the same type in block PHP section
3 participants