Refactor: properly handle callable array even when options are missing #1371
Conversation
Improved handling for cases where `$callable` is an array but does not include the `$callable['options']` key. In such cases, the default options are preserved and no errors occur. Also streamlined the normalization logic by explicitly checking for valid entries (`!empty()`)
|
@matteotrubini can you add some test cases for this? |
Expanded MarkupManagerTest to cover registration and validation of Twig functions, including handling of options, default options, empty callables, and invalid callables.
LukeTowers
added
the
needs review
|
@LukeTowers, once I get your green light, I’ll proceed with makeTwigFilters() as described in the first comment. |
|
@matteotrubini ideally the options logic that's present everywhere in Winter could be centralized to a helper as much as possible. Should be good to proceed with the makeTwigFilters |
matteotrubini
force-pushed
the
markupmanager-makeTwig-callable
branch
from
ed98069 to
4f88f25
Compare
matteotrubini
force-pushed
the
markupmanager-makeTwig-callable
branch
from
39e3d0c to
21c9cbd
Compare
|
I have finished my review and the tests are green. I would squash the commits before marking the PR as ready - what do you think, @LukeTowers? |
|
@matteotrubini I always squash when merging, no need to worry about squashing on the PR itself. |
|
Is the intention behind this to allow definitions using October v4's syntax? (i.e. OFFLINE-GmbH/oc-responsive-images-plugin@741e145, https://github.com/octobercms/october/blob/8b8d2f7444ea98da488014504f51e12f4150465a/modules/system/classes/MarkupExtensionItem.php#L80-L98). If that's the case I think we need to be a bit more explicit about how we're supporting that to make sure that we're also properly parsing the options. |
|
The purpose of this PR is the one described in the first comment, and it was achieved with the initial commit. |
2 participants
Improved handling for cases where
$callableis an array but does not include the$callable['options']key. In such cases, the default options are preserved and no errors occur.Also streamlined the normalization logic by explicitly checking for valid entries (
!empty())makeTwigFilters()function as well for consistency.Eventually, both implementations could be unified into a shared helper, as they follow the same logic.