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.
Child issue of #1929270: [meta] Drupal-agnostic components should not be calling Drupal functions
The issue might be fixed, but adding an automated test is a nice way to avoid regressions
Comment | File | Size | Author |
---|---|---|---|
#40 | interdiff-2282673-38-40.txt | 588 bytes | Anonymous (not verified) |
#40 | drupal-no_core_in_component_test-2282673-40.patch | 8.15 KB | Anonymous (not verified) |
#38 | interdiff-2282673-36-38.patch | 1.28 KB | Anonymous (not verified) |
#38 | drupal-no_core_in_component_test-2282673-38.patch | 8.18 KB | Anonymous (not verified) |
#36 | drupal-no_core_in_component_test-2282673-36.patch | 7.13 KB | Anonymous (not verified) |
Comments
Comment #1
ParisLiakos CreditAttribution: ParisLiakos commentedComment #2
ParisLiakos CreditAttribution: ParisLiakos commented#1825952: Turn on twig autoescape by default
Comment #3
mgiffordThis shouldn't still be postponed I think.
Comment #5
mgiffordReroll of #2.
Comment #6
tim.plunkettThose are just cosmetic fixes, and missing the entire test.
Comment #8
ParisLiakos CreditAttribution: ParisLiakos commentedthanks for reminding me this
Comment #9
jhedstromI agree this seems like a sound way to avoid regressions. Test looks good. Can somebody confirm though that all the cleanup here is still relevant/intended?
Comment #10
znerol CreditAttribution: znerol commentedGreat plan. Test passes when patch applied and fails when I deliberately place a file mentioning the core namespace in the component directory.
Use
===
.Use
strpos(...) !== FALSE
, also the assertion results in a rather unhelpful message. E.g.Failed asserting that 4 is false
. Would it be possible to at least mention the path to the offending file? Maybe even the line number?Comment #11
Anonymous (not verified) CreditAttribution: Anonymous commentedMy eyes couldn't unsee https://twitter.com/da_wehner/status/527128620394094593
Comment #12
Anonymous (not verified) CreditAttribution: Anonymous commentedForgot interdiff.
Comment #13
znerol CreditAttribution: znerol commentedGreat, thanks.
I think it also would be possible to write
foreach (... as $line_number => $line)
.This is about the core namespace, not classes. Perhaps something like Illegal reference to 'Drupal\\Core' namespace in $class_path at line $line_number
Also we need to use
assertNotIdentical(FALSE, strpos(...), ...)
for the same reasons pointed out in the previous review. Sorry that I did not realize that before.Comment #14
Anonymous (not verified) CreditAttribution: Anonymous commented"assertNotIdentical" not found using these test bases - are you implying that function needs to be created as it is in other places like views tests or can I link it in from somewhere?
Comment #16
dawehnerThank you steve!
I guess he was talking about assertNotSame
... as I saw in another issue, we should expand this to do the same for the tests itself, so we ensure that also tests don't use anything from the core namespace.
Comment #17
Anonymous (not verified) CreditAttribution: Anonymous commentedTrying first bit, will look into test test coverage tomorrow.
Comment #18
znerol CreditAttribution: znerol commentedComment #20
Anonymous (not verified) CreditAttribution: Anonymous commentedCurrently unsure as to whether assertNotSame works - got two halves of a dev machine which I need to sort out to test properly locally but posting this in the meantime. Last test failed cos logic swapped back but now can't get it to fail.
Comment #21
Anonymous (not verified) CreditAttribution: Anonymous commented...and of course the second I post I realise what the issue is - strpos can return false but it'll return a posn, not TRUE. Putting the coffee on...
Comment #22
znerol CreditAttribution: znerol commentedShould be
assertSame(FALSE, strpos(...), ...)
. Sorry for the confusion.With this change, the test should detect the following violations in component tests:
Comment #23
znerol CreditAttribution: znerol commentedFunny,
UuidTest
does not seem to use theContainerBuilder
at all. I think this use statement is spurious and can be deleted. The same applies forPhpStorageFactory
inPluginBaseTest
.Also it seems that
PhpStorageTestBase::$storageFactory
is never used, neither in the base class nor in any of the subclasses. That means that this property can be removed, along with thesetUp()
method and the use statement forPhpStorageFactory
.Comment #24
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #25
Anonymous (not verified) CreditAttribution: Anonymous commentedIs this meant for this issue?
goes to get more coffee...
Comment #26
znerol CreditAttribution: znerol commentedAs soon as the newly implemented unit test works correctly, it will detect the violations in the component test namespace and will fail because of that (which is good!). So either we need to open a new issue for fixing the offending tests (
UuidTest
,PluginBaseTest
andPhpStorageTestBase
) and get that one in before, or we can fix them in this issue. I propose to fix them here if possible and only open a new issue if this turns out to be too complex (i.e. requiring functional changes to the tests).Comment #27
Anonymous (not verified) CreditAttribution: Anonymous commentedOIC - sorry, just jumping in so not fully aware of issue scope (feel like I've been doing that for years ;) Thanks, will take a look.
Comment #29
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #30
znerol CreditAttribution: znerol commentedGreat. From local testing I see there is one little thing left. Ironically, the new test detects a core reference in itself. The problem is the comment in line 64.
Change it to something like Asserts that the file is not using any class from Core namespace and we should be fine.
Comment #31
Anonymous (not verified) CreditAttribution: Anonymous commentedD'oh of course! My local dev setup is Yosemite'd at the moment, trying to fix whilst patching manually ;)
text changed
Comment #33
znerol CreditAttribution: znerol commentedPerfect. Thank you for picking that up and good luck with deyosemiting your box.
Comment #34
dawehnerThank you!
Comment #35
Wim LeersHaha! :D
Great patch, thank you!
RTBC+1
A few nits that can be fixed on commit:
s/Definition of Drupal/Contains \Drupal\/
s/Drupal\Component/\Drupal\Component/
We don't add
@group Drupal
anymore.s/php/PHP/
s/file/given class/
Comment #36
Anonymous (not verified) CreditAttribution: Anonymous commentedJust on way out to London - dev machine still broken but quickly manually edited the patch according to #35. Presume 'on commit' may mean this not necessary but thought might help.
Comment #37
alexpottSortArray still needs quite a bit of cleaning up.
I think all the methods here should just have a line which says (if it is)
Or something similar.
I guess someone might argue this is out-of-scope but since we doing:
I think not.
Comment #38
Anonymous (not verified) CreditAttribution: Anonymous commentedNo problem, how's this?
Comment #39
znerol CreditAttribution: znerol commentedOne final nitpick. Instead of removing this line entirely, it could be replaced with the comment now used elsewhere, i.e.
Callback for uasort().
Comment #40
Anonymous (not verified) CreditAttribution: Anonymous commentedKicking myself for missing that!
Comment #41
znerol CreditAttribution: znerol commentedThanks.
Comment #42
alexpottCommitted 914b78d and pushed to 8.0.x. Thanks!
Comment #43
alexpottThis issue is a unfrozen change (testing) as per #2350615: [policy, no patch] What changes can be accepted during the Drupal 8 beta phase? and it's benefits outweigh any disruption.