Problem/Motivation

I find a number of parameters in potx.inc that are called $save_callback, $version_callback or $header_callback.
These are currently documented as `@param string` (with no php type).

For other modules like l10n_server, it would be convenient if these parameters accept any callable.
Technically it seems like this would already work.

Steps to reproduce

Proposed resolution

These parameters should probably be documented as 'callable', to formally allow for closures.
Optionally, the 'callable' type can be added in php.

I don't think if is a BC break, because these values are being used as callables.
Passing something that is not callable would already break today.

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork potx-3577849

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

donquixote created an issue. See original summary.

donquixote opened merge request !42

donquixote’s picture

donquixote commented
Status: Active » Needs review

I think for now we should do only the doc change, not the php signature change which is in the second commit.
But I want to see how tests behave.

donquixote’s picture

donquixote commented

Actually, more important than the doc change could be to add a test to verify that callables are accepted :)

donquixote’s picture

donquixote commented

I replaced all the literal string function arguments with first-class callable.
I don't see any failures.

I still think we should do the simplest version of this MR which is just the first commit.

As for tests:
We do have tests where we call these functions, currently with string arguments.

To truly cover that we can pass any callable, we would now have to have some tests where we pass string, and others where we pass closure or even other callable types.
One way to achieve this would be to run the same test two times, one time with first class callable and one time with string function names. But really?

The other option would be to create additional tests that are simpler, where we only pass locally created closures, and nothing is really saved.
This seems like it could be a good idea, but I currently only scratching the surface with knowledge of potx module. I would need to better understand the contract that these functions have.

donquixote opened merge request !43

donquixote’s picture

donquixote commented

Another step we could do is to document the signature of the callables.
I did that in one place in the "3577849-pass-callables-everywhere" branch, but I don't know if we really want to go there at this point.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs review » Fixed

Merged the docs suggestion.

Re the actual first class callable syntax I think that would be fine if potx can require PHP 8.1 (per https://www.php.net/manual/en/functions.first_class_callable_syntax.php) Currently the info file says potx works down to Drupal 8. We can't automatically test that on drupal.org anymore, so probably should not claim that either, but that should be a conscious decision. (And could potentially lead to some cleanup too).

I'm inclined to close this issue and we can revisit the more involved change later. We need localize.drupal.org updated to Drupal 11 first to finally let go of Drupal 7 potx and refactor potx for modern Drupal for real. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

donquixote’s picture

donquixote commented

Thanks for merging!

Re the actual first class callable syntax I think that would be fine if potx can require PHP 8.1

I'm inclined to close this issue and we can revisit the more involved change later.

Ok for me.
I mostly created that second MR to see if it would break anything.
For now it is just useful to know that non-function-string callable is officially supported.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.