Closed (fixed)
Project:
Drupal core
Version:
8.0.x-dev
Component:
base system
Priority:
Critical
Category:
Bug report
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
14 Oct 2015 at 12:23 UTC
Updated:
28 Oct 2015 at 13:44 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
alexpottWe need to investigate why DrupalCI did not report this but let's get the quick fix in first.
Comment #3
wim leersHow is it possible that tests did not fail for #2585165: Don't include vendor test code (especially mink) in the Drupal webroot's patch?
Comment #4
alexpott@Wim Leers I think this is because PHPUnit failed during class creation...
class ContainerAwareEventDispatcherTest extends AbstractEventDispatcherTest. I think there is an issue for this.Comment #5
dawehnerCan't we simply copy over the existing abstract test class and be done with it?
Comment #6
alexpott@dawehner I'd rather get the fix upstream and remove the dependency - we need to copy quite a few of the test fixtures and we have no idea if they might add more in the future.
Comment #7
alexpottThe fail is in the logs for #2585165: Don't include vendor test code (especially mink) in the Drupal webroot see https://dispatcher.drupalci.org/job/default/23686/consoleFull
So at least we can use DrupalCI to prove this works.
Comment #8
alexpottSo DrupalCI shows this is fixed... https://dispatcher.drupalci.org/job/default/23819/consoleFull
I think this is a good quick stopgap to the better EventDispatcher getting in upstream.
Comment #9
alexpottMaybe this would have been caught if #2582457: Failed PHP Unit tests should report detailed information (regression) had landed. I pressed re-test over there.
Comment #10
dawehnerEven I'm still convinced that copy over the one file would solve the same issue, and would be better this patch looks okay.
It is a quickfix so, let's be pragmatic.
Comment #11
dawehnerWell, as it turns out, the begging was not necessarily 100% convinced
Comment #12
alexpottI tried to do what @dawehner asked but I ended up not being to just re-namespace
abstract class AbstractEventDispatcherTestbecausetestAddSubscriberWithPrioritiesdoes an instanceof check. So I thought lets override that in theContainerAwareEventDispatcherTestbut that didn't work because$dispatcheris private.Therefore I think the current patch is (unfortunately) the way to go here. Hopefully we can remove all of this duplication when https://github.com/symfony/symfony/pull/12521 lands.
Comment #13
catchCommitted/pushed to 8.0.x, thanks!
Comment #15
MixologicA thousand pardons. I'll spend time on #2580293: Patch having test with "PHP Fatal error" is marked as PASSED today - this normally would not happen to phpunit tests, except, in this case since the class it was extended from was removed, it had no way of knowing it was a phpunit test.