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 8000 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
Merged
merged 10 commits into from
Jun 9, 2025
Merged

Correctly support the default callback on tagged services #8361

merged 10 commits into from
Jun 9, 2025

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
8000 @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.

@leofeyer
Copy link
Member

No, the PR adds support for arrays by wrapping them into a closure (see line 73).

@fritzmg
Copy link
Contributor
fritzmg commented May 26, 2025 •
edited
Loading

Yes - what I mean is: even if this PR is merged, you still cannot do this:

'default' => [MyService::class, '__invoke'],

i.e. the PR does not add inherent support for arrays for the default callback.

@leofeyer
Copy link
Member

It does not? But there is a unit test for exactly this case 🤔

@fritzmg
Copy link
Contributor
fritzmg commented May 26, 2025 •
edited
Loading

The unit test only tests the compiler pass for the DataContainerCallbackListener - but the actual implementation there wraps the array into a closure for this reason.

The implementation in DC_Table is untouched - i.e. DC_Table still only supports values and closures for default, no arrays.

@aschempp
Copy link
Member Author

yes, it does not change DC_Table. But service callbacks are always arrays, which are not supported here. So we must wrap them to actually support them.

@aschempp aschempp force-pushed the fix/default-callback branch from 4af806c to 82b6ef8 Compare May 29, 2025 09:02
@aschempp
Copy link
Member Author

I have added three more options in 82b6ef8. Currently, the callback registering tries to be clever to automatically detect stuff. However, that is sometimes problematic. Here are the tree options:

  1. exact attribute can be used to prevent appending _callback to the target name. There are callbacks that do not follow the naming scheme, and they the supported list is hardcoded.
  2. closure attribute can be used to enforce a closure, same as we need for the default callback
  3. singleton enforces a single callback on the given target key, because the current singleton list is hardcoded.

We could argue these are new features, let me know what you think. I can rebase the whole PR or just these options. But imho the current state is kinda broken because you cannot register all cases through the service container, which I just ran into with MultiColumnWizards columnCallback (camel case instead of underline).

@aschempp aschempp requested a review from leofeyer May 29, 2025 09:18
@leofeyer leofeyer added the up for discussion Issues and PRs which will be discussed in our monthly calls. label May 30, 2025
@leofeyer leofeyer removed the up for discussion Issues and PRs which will be discussed in our monthly calls. label Jun 5, 2025
@leofeyer leofeyer merged commit eb58b1e into contao:5.3 Jun 9, 2025
18 checks passed
@leofeyer
Copy link
Member
leofeyer commented Jun 9, 2025

Thank you @aschempp.
8000

@aschempp aschempp deleted the fix/default-callback branch June 10, 2025 06:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leofeyer leofeyer leofeyer approved these changes

@Toflar Toflar Toflar left review comments

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