Problem/Motivation
This has become a problem for Drush, see #1930. Drush replaces the batch context array with a classed object to be able to output the message to the command-line when a callback sets it. Due to the use of ArrayObject
that works fine with code getting and setting values in the array syntax. However, callbacks that typehint $context
as an array
break with Drush. Due to improved error handling this just recently surfaced, previously the error was being silenced.
Proposed resolution
Even though it is not a bug in Drupal itself I think we could be nice to Drush and potentially others and remove the type-hints. This is also in line with other places in core where we have moved to being forward-compatible with classed objects and have removed array typehints. The batch context in particular is well-suited for such a classed object, maybe even in 8.1 core.
Even though this is not a bug in Drupal core, it is effectively causing a bug in Drush which we could easily fix here so targeting 8.0.
Remaining tasks
Task | Novice task? | Contributor instructions | Complete? |
---|---|---|---|
Create a patch | Instructions | ✓ | |
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards | Instructions |
User interface changes
None.
API changes
None. We could perhaps write a change notice to encourage contrib module authors to remove array typehints as well, but we're not breaking anyone's code.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#17 | 2664290-17.patch | 5.31 KB | tstoeckler |
#9 | 2664290-9.patch | 7.65 KB | tstoeckler |
Comments
Comment #2
tstoecklerVerified that with this
drush locale-check
starts working again.Comment #3
tstoecklerComment #4
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedJust a quick note -- while I have not taken a look at the batch processing code in Drupal 8 at this point, it would also be an option for Drush to use a different mechanism to receive batch messages from Drupal core, if any such mechanism exists.
If there is no such thing available, I think it would be best to go ahead and remove the typehints. It is too bad that there is no way to typehint `array` or `ArrayObject` in PHP. It would be possible to test for `is_array()` or `instanceof ArrayObject` at runtime, if desired; this would work fine for Drush.
Comment #5
tstoecklerFor anyone that is - like me - wondering why
drush config-import
currently isn't broken despite the typehints: Drush doesn't use a batch for importing the configuration but callsConfigImporter::import()
directly. That then sets$context
to an array.Comment #6
moshe weitzman CreditAttribution: moshe weitzman at Acquia commentedThe change makes sense to me, and fixes the error.
Comment #7
rodrigoaguileraIt applied the patch so I can solve the error and it works indeed.
Comment #8
catchDrupalCS/Coder will complain about the missing type hints, these should probably all be @param mixed $foo - and should it say array/ArrayAccess?
Comment #9
tstoecklerI think that's a good idea. Also found one function I weirdly overlooked. (Btw, I grepped for
array $context
to find these, so in theory if there's a batch callback that named it's variable differently I would have missed it, too.)Comment #10
dawehnerHow do we deal with protected functions changing their signature? Theoretically that class might have been extended?
Comment #11
catch@dawehner so you get E_STRICT for < PHP 7, and E_WARNING for PHP 7.
I think this is fine for a minor release, not for a patch, so moving to 8.1.x
Comment #12
tstoecklerRe #10/#11: Can we at least get the locale changes into 8.0? I don't see any way how that could break anything and - as mentioned in the IS - is actually a bug fix (albeit not for core, but for Drush).
Comment #13
moshe weitzman CreditAttribution: moshe weitzman at Acquia commentedWell lets start with 8.1
Comment #16
catchCommitted/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!
If you re-roll for backport, please re-open against 8.0.x.
Comment #17
tstoecklerThis is the same patch without the
ConfigImporter
changes, so as far as I know, this is 100% backwards-compatible.Comment #18
tstoecklerBump. Even though 8.0 is on its way out I think this should go into 8.0 because this makes working with Drush on 8.0 sites a pain. And even once 8.1 comes out, people will still have to work on 8.0 sites for a while...
Comment #19
dawehnerYeah I agree with the point made by tobias.
Comment #20
vasi CreditAttribution: vasi at Evolving Web commentedThis seems to do a great job on 8.0.x.
Comment #21
alexpottCommitted 212c73c and pushed to 8.0.x. Thanks!