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

Contributor tasks needed
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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler created an issue. See original summary.

tstoeckler’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
6.55 KB

Verified that with this drush locale-check starts working again.

tstoeckler’s picture

Issue summary: View changes
greg.1.anderson’s picture

Just 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.

tstoeckler’s picture

For 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 calls ConfigImporter::import() directly. That then sets $context to an array.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

The change makes sense to me, and fixes the error.

rodrigoaguilera’s picture

It applied the patch so I can solve the error and it works indeed.

catch’s picture

Status: Reviewed & tested by the community » Needs work

DrupalCS/Coder will complain about the missing type hints, these should probably all be @param mixed $foo - and should it say array/ArrayAccess?

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
4.99 KB
7.65 KB

I 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.)

dawehner’s picture

How do we deal with protected functions changing their signature? Theoretically that class might have been extended?

catch’s picture

Version: 8.0.x-dev » 8.1.x-dev

@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

tstoeckler’s picture

Re #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).

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Well lets start with 8.1

  • catch committed 6e5de2b on 8.2.x
    Issue #2664290 by tstoeckler: Remove array typehints from batch...

  • catch committed 63eba07 on 8.1.x
    Issue #2664290 by tstoeckler: Remove array typehints from batch...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/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.

tstoeckler’s picture

Version: 8.1.x-dev » 8.0.x-dev
Status: Fixed » Needs review
FileSize
5.31 KB

This is the same patch without the ConfigImporter changes, so as far as I know, this is 100% backwards-compatible.

tstoeckler’s picture

Bump. 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...

dawehner’s picture

Yeah I agree with the point made by tobias.

vasi’s picture

Status: Needs review » Reviewed & tested by the community

This seems to do a great job on 8.0.x.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 212c73c and pushed to 8.0.x. Thanks!

  • alexpott committed 212c73c on 8.0.x
    Issue #2664290 by tstoeckler: Remove array typehints from batch...

Status: Fixed » Closed (fixed)

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