8000 Correctly support the default callback on tagged services by aschempp · Pull Request #8361 · contao/contao · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Correctly support the default callback on tagged services #8361

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 5 commits into
base: 5.3
Choose a base branch
from
Open

Correctly support the default callback on tagged services #8361

wants to merge 5 commits into from

Conversation

aschempp
Copy link
Member

see #8340

@aschempp aschempp added this to the 5.3 milestone May 21, 2025
@aschempp aschempp requested a review from a team May 21, 2025 06:56
@aschempp aschempp self-assigned this May 21, 2025
@aschempp aschempp requested a review from leofeyer as a code owner May 21, 2025 06:56
@aschempp aschempp added the bug label May 21, 2025
Toflar
Toflar previously approved these changes May 21, 2025
View reviewed changes
leofeyer
leofeyer requested changes May 21, 2025
View reviewed changes
Copy link
Member
@leofeyer leofeyer left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default field does not work the same way as the callback fields. It only supports a value or an instance of Closure:

contao/core-bundle/contao/drivers/DC_Table.php

Lines 738 to 745 in e55f40a

$default = $v['default'];
if ($default instanceof \Closure)
{
$default = $default($this);
}
$this->set[$k] = \is_array($default) ? serialize($default) : $default;

And an array with object and method does not match this requirement: https://3v4l.org/QKX9q It would simply be serialized to a string.

@aschempp
Copy link
Member Author

Should we fix this to support an array like any other callback in the DCA then?

@leofeyer
Copy link
Member

That would be a new feature then, wouldn't it?

@aschempp
Copy link
Member Author

Yes and it would conflict with currently allowed values. 680120c should fix this by wrapping in a closure.

@leofeyer
Copy link
Member

I'm not sure about his. Why do we have to handle arrays at all costs? Value or closure has worked fine so far. @contao/developers /cc

@fritzmg
Copy link
Contributor
fritzmg commented May 23, 2025

I think for consistency all DCA callbacks should work the same. But the current state of this PR doesn't change this anyway, does it? For default still only closures are supported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leofeyer leofeyer leofeyer requested changes

@Toflar Toflar Toflar left review comments

Requested changes must be addressed to merge this pull request.

Assignees

@aschempp aschempp

Labels
bug
Projects
None yet
Milestone
5.3
Development

Successfully merging this pull request may close these issues.
4 participants
@aschempp @leofeyer @fritzmg @Toflar
0