Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Right now we are injecting classes TwigExtension requires using setter methods.
Proposed resolution
Remove all the setters and inject required services by using constructor
Comment | File | Size | Author |
---|---|---|---|
#26 | remove_twigextension-2571633-26.patch | 17.36 KB | tuutti |
#26 | interdiff-2571633-26.txt | 445 bytes | tuutti |
#24 | remove_twigextension-2571633-24.patch | 17.36 KB | tuutti |
#24 | interdiff-2571633-24.txt | 5.85 KB | tuutti |
#22 | remove_twigextension-2571633-22.patch | 17.32 KB | tuutti |
Comments
Comment #4
joelpittetIs this still needed? Constructor injection requires more to mock when PHPUnit testing.
This would currently be 4 new constructor parameters and more if we add more services providing twig filters and functions.
Comment #5
lauriiiThis is definitely still needed. It is bad practice to use setter methods for required dependencies. Caller code doesn't expect that it has to call setter methods to provide needed dependencies and therefore will cause easily unexpected errors if they're not supplied.
Comment #6
lauriiiComment #8
lauriiiSomething went wrong with the previous patch.. Luckily PHPStorm local history saved me..
Comment #10
lauriiiRe-uploading for 8.3.x
Comment #12
lauriiiComment #14
lauriiiComment #15
martin107 CreditAttribution: martin107 as a volunteer commentedThe change is a nice cleanup/standardization.
Unless there is a specific reason it is better to be more tightly constrained - inject at the point of construction and be done with it .. rather than allow the dependencies to be set dynamically.
I have visually scanned the patch ... all changes make sense.
Comment #16
joelpittetI'm +1 for this now talking to @laurii and @xano because it makes sense that all of our deps are not optional.
this is 'system' under testing not 'service' as an acronym but maybe this could be a spelled out variable? The typo can be changed on commit if it's ok to keep that variable name.
Comment #17
alexpottI don't think we can remove these in Drupal 8. The rest of the patch looks fine for D8 - we've agreed that constructors can change in minor versions.
Comment #18
lauriiiYes sir!
Comment #19
lauriiiComment #20
joelpittetThanks for the changes @lauriii and review @alexpott
Comment #21
alexpottDoes sut mean system under test or a service under test? I think we should just be descriptive here. Also the problem here is that system under test and unit testing concepts are tricky to define to together. Looking at http://xunitpatterns.com/SUT.html. The test needs to clearly define what it means by system. But just introducing an acronym makes it more difficult to understand for people. For no reason we are making people have to think "What does sut mean and what does it stand for" when the more valuable considerations are does the test test what it claims to.
Comment #22
tuutti CreditAttribution: tuutti at KWD Digital commentedComment #23
joelpittetThe problem isn't the concept of 'system under test' it's just the variable being short. Thanks for trying to resolve @tuutti! I did overhear @lauriii and @alexpott discussing this at the table yesterday at Badcamp, and it just needs a long form name from what I gathered.
Maybe
$systemUnderTest
?Comment #24
tuutti CreditAttribution: tuutti at KWD Digital commentedLets try again!
Comment #25
tuutti CreditAttribution: tuutti at KWD Digital commentedComment #26
tuutti CreditAttribution: tuutti at KWD Digital commented...changed code comment to match the variable name.
Comment #27
joelpittetThank you for addressing that @tuutti.
Comment #28
alexpottCommitted dd79f79 and pushed to 8.3.x. Thanks!
Unused uses fixed on commit.
Minor improvement on commit.
Comment #30
alexpott