Closed (fixed)
Project:
Drupal core
Version:
8.2.x-dev
Component:
simpletest.module
Priority:
Normal
Category:
Task
Assigned:
Issue tags:
Reporter:
Created:
7 Apr 2016 at 22:01 UTC
Updated:
24 Mar 2017 at 09:17 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
larowlanComment #3
larowlanComment #4
dpopdan commentedComment #5
dpopdan commentedComment #7
larowlanWe need to leave the old classes there, just as stubs.
Comment #8
dpopdan commentedUnfortunately the classes were modified since my last patch, so it won't apply correctly and I am not sure that an interdiff would help in this case. @larowlan I am not sure that I understood correctly what you mean with keep them as stub, can you explain it better please ?
Comment #9
dpopdan commentedComment #10
dawehnerIMHO waiting for #2469721: Add functionality to store browser output to BrowserTestBase would make a bit of sense ...
One question about the patch, why does it not remove some code?
Comment #12
Torenware commentedWould you consider adding UserCreateTrait as well? Or does that need to be a different issue? Looks to be a very simple port, since there aren't much in the way of dependencies on SimpleTest, it turns out. See #2690403: Fatal error: Class 'Drupal\field\Tests\FieldUnitTestBase' not found
Comment #13
larowlan@Torenware - thats a new issue - but feel free to post link to it here to illicit some reviews.
Comment #14
dawehner.
Comment #15
edurenye commentedRerolled.
Do we need to do it also for 8.1.x?
Comment #16
dawehnerI doubt this will land in 8.0.x or 8.1.x now, so maybe deprecated as of 8.2.x ...
Comment #17
edurenye commentedSet deprecated in Drupal 8.2.0
Comment #18
dawehnerCool, thank you!
Comment #19
alexpottThe deprecated traits should be gutted and use the new traits so we don't have code duplication - which is exactly what traits are supposed to help prevent :)
Comment #20
Torenware commented@alexpott --
This is OK only as long as you are willing to create some real pain for people writing tests for Contrib modules.
The test bot is set up to use the current dev version, which is now 8.2.x. But people who actually use contrib modules are using 8.1.x now. If you remove the old traits from the 8.2 tree now, then THERE IS NO SIMPLE WAY TO WRITE A TEST THAT USES THESE TRAITS THAT WILL RUN ON 8.1 AND STILL PASS THE TEST BOT. You are effectively forcing people to rewrite their tests to not use these traits. So the code duplication you are ostensibly preventing in Core is going to be forced onto Contrib people. That, or you are effectively asking Contrib people to maintain multiple versions of their 8.x modules, which is ridiculous.
This does not strike me as good for the community. Core does not exist for its own sake. Core exists so people can use it.
Please consider the possibility that a little code duplication in Core might do one hell of a lot to prevent code duplication in a lot of other projects by a lot of other teams.
Comment #21
larowlanworking on this
Comment #22
larowlan"The deprecated traits should be gutted and use the new traits"
Comment #23
dawehnerThe patch says these classes will be removed from 9.0.x, so the concerns of #20 should be addressed
Comment #24
alexpott@Torenware I think you need to reconsider the -- you gave me in #20 as @larowlan has shown that I didn't want to create pain for anyone. In fact now everyone wins - we have the traits outside of Simpletest, the old traits still work and we only have to maintain the code in one place.
Comment #25
alexpottDiscussed with @dawehner and we couldn't see a reason to not do this in 8.1.1 as maintaining a single core test code base makes life simpler and there are no bc breaks here.
I was going to commit this but then there were lots of unused uses and an unnecessary trait name replacement.
Comment #26
dawehnerThank you alex! Still RTBC for me
Comment #27
alexpottCommitted a1c9c81 and pushed to 8.1.x and 8.2.x. Thanks!
Comment #30
rosinegrean commented