Problem/Motivation
debug()
can be helpful from time to time. Sadly its throwing an error which phpunit converts to an exception, which stops the processing of anything.
Proposed resolution
Allow Symfony's VarDumper component's dump() function to be used in tests as follows:
- Unit tests: Allow dump() to work in tests run in isolation. (dump() already works in these if the test is not run in isolation, which is the default for Unit tests).
- Kernel tests: allow dump() to work in both test code and SUT code
- Functional tests: dump() already works in SUT code (output is shown in HTML output from the SUT site). Allow dump() to work in test code
Remaining tasks
Do it.
User interface changes
None.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|
Issue fork drupal-2795567
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
dawehnerComment #4
joachim CreditAttribution: joachim commentedDoesn't this count as a regression, since it makes it harder to debug tests?
You can use debug() in a BrowserTestBase, but you can only use it once because the test stops there.
I've used Symfony's dump() (from symfony/var-dumper) in PHPUnit tests I've written for non-Drupal code (Dorgflow and Drupal Code Builder), and dump() works *great* in PHPUnit, and looks good on the command-line too. Would that be a viable alternative?
Comment #5
joachim CreditAttribution: joachim commentedPoking around at this a bit...
It looks like we need TestHttpClientMiddleware to dump() any debug()ed output it finds in an X-Drupal-Assertion HTTP header, rather than throw an exception.
However, there's something about our tests that doesn't like dump(): this is the result of doing dump() in the actual test class code:
This is not at all what happens when I use dump() in my other projects! Is this just because Drupal is using a much older version of PHPUnit, or is Drupal doing something weird here?
Comment #6
dawehnerI think the problem is that Drupal is using the process isolation feature of phpunit.
Comment #7
joachim CreditAttribution: joachim commented> I think the problem is that Drupal is using the process isolation feature of phpunit.
I think that's where the problem is then -- any kind of output during the test causes a crash.
The process for getting data from a call to debug() in module code to the test system via HTTP headers and then the TestHttpClientMiddleware class is already there. It could definitely use some improvement, and I think using Symfony VarDumper would be good, but that should probably be a separate issue.
Comment #8
dawehnerOh yeah Symfony's var dumper is truely a great piece of functionality. It changes things night and day.
I wonder whether we can somehow pass information down to the parent test process without failing. Maybe we could leverage some of the pieces in phpunit which allows to restore global behaviour.
Comment #9
joachim CreditAttribution: joachim commentedHmm yeah, using dump() in one of Flag's unit tests works fine. So it's definitely a problem with BrowserTestBase...
Comment #11
joachim CreditAttribution: joachim commentedI think the trick is going to be to use a custom dumper with Symfony's var-dumper (see https://symfony.com/doc/current/components/var_dumper/advanced.html).
We can have that write successive dump lines somewhere save (like the Drupal state? or the session?) and then retrieve them at the end of the test.
Comment #12
joachim CreditAttribution: joachim commentedHmm, stuffing dump lines into the state works, but at what point do we leave the PHPUnit child process so we can get them out?
Comment #13
joachim CreditAttribution: joachim commentedHere's a quick and dirty patch which uses Symfony's dumper and sends the output out to a file, which you can 'tail -f' in another terminal window.
You sadly don't get the coloured output that the dumper would normally produce, but it's already a big improvement, as the whole test runs rather than crashes at the first dump() statement!
Comment #17
Sophie.SKIt's possible to do some debugging using
fwrite
:however this isn't an ideal, permanent solution. I've not been able to print full objects/arrays/etc - would be good to see this combined with the work Joachim has already done somehow as a lot of posts on t'interwebs seem to point to
debug()
being the best way forward.Comment #18
joachim CreditAttribution: joachim commented@Sophie.SK's fwrite(STDOUT trick works great here -- it somehow is bypassing PHPUnit's monitoring, so PHPUnit doesn't complain that the test is failing because of the output.
It's even possible to get coloured output!
The indentation for object properties seems to be missing, but still, a huge improvement.
Comment #20
joachim CreditAttribution: joachim as a volunteer commentedI've figured out the indents!
(No thanks to Symfony's total lack of code documentation...)
Comment #22
joachim CreditAttribution: joachim as a volunteer commentedAh.... symfony/var-dumper isn't installed with Drupal!
Not sure how to handle this.
Quick and dirty is to stick a class_exists() around the whole thing?
Comment #23
joachim CreditAttribution: joachim as a volunteer commentedComment #24
daffie CreditAttribution: daffie commentedBig +1 for me.
Comment #25
joachim CreditAttribution: joachim commentedJust tried the same trick in BrowserTestBase and it works too!
When I have some free time, I'll refactor the patch into something like a common helper class rather than anonymous functions.
Comment #26
joachim CreditAttribution: joachim as a volunteer commentedNew patch:
- added this to functional tests
- moved all the code to a helper class
- added class_exists() so this only runs when symfony var dumper is present.
Comment #27
daffie CreditAttribution: daffie commented@joachim: Your patch looks good. I am missing the functional tests.
We are adding some new functionality, therefore we should add a change record. I am not completely sure how to use the new debug method.
Comment #28
joachim CreditAttribution: joachim commentedDo we need change records for new functionality? I thought it was mostly when an existing API is changed.
> I am not completely sure how to use the new debug method.
Install Symfony Var-Dumper with `composer require symfony/var-dumper`. Then use the dump() statement anywhere -- it can be in both test code and normal code that is run during the test.
Comment #29
daffie CreditAttribution: daffie commentedOK, removing the needs change record tag.
Should this library not be added to Drupal Core?
Comment #30
joachim CreditAttribution: joachim commented> Should this library not be added to Drupal Core?
No, it would only be used for development.
I suppose we could add it to require-dev, but that's a call for maintainers.
Comment #31
joachim CreditAttribution: joachim as a volunteer commentedI'm going to set this back to needs review to get the attention of maintainers.
Comment #32
AaronBaumanSeems like it doesn't really work consistently.
Seem to be quietly failing to output anything for very large variables, and I don't really know why.
Potentially due to some limitations in the test framework or inside var-dumper, which I'm not even gonna attempt to debug.
It's definitely better than what we have now: nothing.
This is a lifesaver.
+1 RTBC
Comment #33
joachim CreditAttribution: joachim as a volunteer commented> Seem to be quietly failing to output anything for very large variables, and I don't really know why.
How large? I've dumped entities, which are pretty large what with all their internal bits. Also, I've dumped things that are too long for my terminal's scrollback, such as objects that had the extension manager injected, with a HUUUUUGE list of ALL the module info objects.
Comment #34
AaronBaumanMaybe i'm not using it correctly then.
I'm calling
dump()
on some very simple entities and there's no output.Also seems like only the first call to
actually gets written to stdout, and testbot counts it as an error when that happens.
Here's the command I'm using to run tests:
sudo -u _www php core/scripts/run-tests.sh --color --keep-results --concurrency "31" --url "https://d8.dev" --sqlite "/tmp/foo.sqlite" --dburl "mysql://root@127.0.0.1/d8" --verbose --class 'Drupal\Tests\example\TestClass'
Comment #35
joachim CreditAttribution: joachim as a volunteer commentedAh, I don't use core/scripts/run-tests.sh to run tests -- I've never managed to figure it out.
I use vendor/bin/phpunit, doing something like:
With that, multiple dump() statements work and they don't get counted as an error.
It sounds like this patch has no effect on run-tests.sh, as the use of dump() being counted as a test error is the problem it fixes.
Comment #36
AaronBaumanWow, thanks that's a much cleaner way to run the tests.
When i call phpunit directly, dump() works as described.
Comment #38
Sophie.SKRe-rolling for latest 8.9.
I can't, for the life of me, get this interdiffed. I don't know what my computer's doing, but the only small changes are line numbers in the use statements on KernelTestBase.
Comment #39
Sophie.SKApparently that one wasn't good for 8.8.1, so re-rolling. This patch has been re-rolled against Drupal 8.8-dev. Ah, line number changes.
Comment #40
jhedstromThis is looking good! I think a quick test within
KernelTestBaseTest
(and perhapsBrowserTestBaseTest
too) would be good to ensure expected behavior.It looks like this file slipped in by mistake.
Comment #41
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedI have re-rolled patch #39.
Comment #42
dsdeiz CreditAttribution: dsdeiz commentedYeah, #41 works really good when testing. For manual tests, I've done:
1.
composer require symfony/var-dumper
as suggested at #28.2. And tested it using
vendor/bin/phpunit web/core/modules/user/tests/src/Kernel/UserRoleEntityTest.php
.Result is something like this:
Comment #43
daffie CreditAttribution: daffie commentedPlease ignore this comment. Mixed up the naming of my patch files.
Comment #44
daffie CreditAttribution: daffie commentedAdded the Symfony component var-dump as require-dev and added testing.
Comment #48
daffie CreditAttribution: daffie commentedComment #49
daffie CreditAttribution: daffie commentedSomehow the file core/tests/Drupal/Tests/TestVarDumper.php did not get added to the patch file.
Comment #50
daffie CreditAttribution: daffie commentedReady for a review.
Comment #51
mondrakeParenting - this may help get rid of the
verbose
legacy test method.Comment #53
jungleAdding the "Deprecated assertions" tag to add this into the kanban board https://contribkanban.com/board/Deprecatedassertions
Comment #54
jungleI'd suggest opening a new issue to add symfony/var-dumper as a dev dependency in core first.
Comment #55
jungleFiled #3164349: Add symfony/var-dumper as a top-level dev dependency
Comment #56
jungleWell, symfony/var-dumper is in core already, should we add it as a top-level dev dependency? Rescoped #3164349: Add symfony/var-dumper as a top-level dev dependency.
Comment #57
jungleRerolled from #49
Comment #59
jungleFixing the failed test.
Comment #60
mondrakeLooks very good. Some points:
1) We should take the opportunity and deprecate Drupal\KernelTests\AssertLegacyTrait::verbose and replace with this.
2)
this is nice as it ensures that calls to
dump
within runtime code get rendered in the HTML. However, the test seems too strict to me, since if anything changes in SF rendering of the dump and/or in theDrupal\user\Entity\Role
, the test will start failing. Maybe we can just check if<span class=sf-dump-note>
is present in the page.I think we have a better namespace for this now,
Drupal\TestTools
This entire test seems irrelevant to me - it will test the content of the dump structure without printing anything on the CLI, which does not mean a lot, plus suffers from the same problem above
3. So we do not have automated test coverage for the fact that the usage of
dump()
in test code actually prints anything on CLI. Doing a manual testing adding the below and running LoggingTestyields
which is what we expect, I guess.
Comment #61
jonathanshawPhpunit can test CLI output, see https://phpunit.readthedocs.io/en/9.2/writing-tests-for-phpunit.html#writing-tests-for-phpunit-output-examples-outputtest-php.
So maybe we can add an automated test for #60.3
Comment #62
mondrakeBTW we may also want to deprecate
debug()
in common.inc, just usingdump()
seems much better. Also, will help remove in D10 from runtime code a function that is only used for development (hopefully...)Comment #63
jonathanshawIf the component is an explicit dependency then class_exists is not needed.
If is is not an explicit dependency, then we can't be sure of the version being used and therefore can't be certain there is a setHandler method or what it's signature is; in which case maybe this should be wrapped in a try/catch.
Comment #64
joachim CreditAttribution: joachim commented> If the component is an explicit dependency then class_exists is not needed.
It'll be a dev dependency once #3164349: Add symfony/var-dumper as a top-level dev dependency is in, so we can remove the class_exists().
Our tests now assume the component is there, since there are tests that cover using dump().
Really great to see this moving forward! Thanks everyone!
Comment #65
jungleThanks @mondrake, @jonathanshaw and @joachim for reviewing!
Re #60.1, not sure if we can do this, simply calling
dump()
in tests, PHPUnit reports errors. For instance:And if we are going to do this, I'd suggest doing it in a seperate issue.
#60.2 and 3 addressed, but not exactly same. See the code.
It's hard to assert colored output, so used
expectOutputRegex()
, instead ofexpectOutputString()
expectOutputString()
neither.Drupal\Test\TestVarDumper
toDrupal\TestTools\DrupalVarDumperHandler
, divided the old handler into two handlers:cliHandler()
forKernelTestBase
andhtmlHandler()
forBrowserTestBase
Comment #66
jungleAdding two screenshots after removing/commenting all expectOutputRegex() calls in Drupal\KernelTests\KernelTestBaseTest::testVarDump and running the test.
Comment #67
jonathanshawNice!
I agree with #60.2.3 that this is too strict. We don't really care about the the properties of the Role, and we don't want this test to break if one of these properties gets removed or gets a new default value. It should be sufficient to test just a couple of properties like id, label, and permissions.
Nothing else in TestTools has its name prefixed with Drupal, might be useful to explain your reasoning here in a comment on this issue.
#3164349: Add symfony/var-dumper as a top-level dev dependency is RTBC by subsystem maintainer so removing tag added in #30 regarding that.
We now have automated tests for the dump output so removing the manual testing tag also.
Comment #68
jungleThanks @jonathanshaw for reviewing again!
testVarDump()
in bothKernelTestBase
andBrowserTestBase
Drupal\TestTools\TestVarDumper
instead.Comment #69
jungleShould not use both `It is` and `it's`, using one of them is better to be consistent. Addressing.
Comment #70
jungleSelf-review:
Previously,
fwrite()
used, and I think it should be used still. So that PHPUnit won't throw an exception, this is what we expected written in IS.Setting back to NW.
Comment #71
jungleWe can not assert output from
fwrite()
by usingexpectOutputRegex()
But after adding
dump()
calls in the test, if the test passes, then the test is sufficient to me -- by default, PHPUnit treats output fromdump()
in tests an exception.Comment #72
jungleAddressing #70, #71, and attaching a screenshot of running the
Drupal\KernelTests\KernelTestBaseTest::testVarDump()
with twodump()
calls inside.Comment #73
jungleI think it's necessary to have a CR still.
dump()
calls are allowed in Kernel Tests, Functional Tests and Functional Javascript Tests. Which is a new feature and we want module developers to know and to use.Comment #74
jungleLet's do them in separate issues. Tagging "Needs followup"
BTW,
debug()
might use a custom VarDump handler as KernelTestBase and BrowserTestBase, If so,Drupal\TestTools\TestVarDumper
should be moving to other namespace probably if necessary. That's why I renamed it toDrupalVarDumperHandler
previously.Comment #76
jonathanshawI think a test should fail or be marked as risky after execution with "This test printed output" if it invoked dump(). This helps to guard against accidentally leaving debug calls in code. If people really want the dump to stay permanently, they can opt out of phpunit's beStrictAboutTestsThatPrintOutput.
What's the behaviour with the patch as it is?
Comment #77
mondrakeI think we need to be clear what would be the effects of using
dump()
in the various contexts:1) use of
dump()
in the SUT (i.e. in the runtime code) in Functional and FunctionalJavascript tests - where is the output going?2) the same for Kernel tests - both when run in isolation and not
3) what would happen if a dump() is called in the context of a Unit or Build test?
4) use of
dump()
in the test code - where is the output going, both in isolation and notSo to be aligned on scope and what can be tested or not
Comment #79
joachim CreditAttribution: joachim commentedMade an issue fork and a new branch, 2795567-symfony-dump-kernel-browser-tests.
- committed the most recent patch at #72
- changed the kernel test so that the dump() output is tested, and so it doesn't actually output anything (which could cause problems for test runners that look at output, and also surprise/confuse developers)
Comment #80
joachim CreditAttribution: joachim commentedI can't get BrowserTestBaseTest::testVarDump() to work.
The call to $body = $this->drupalGet('test-page-var-dump'); is getting a page that's redirecting to install.php.
I tried adding $this->drupalGet('test-page'); to see if an existing test page works, and that does it too.
BrowserTestBaseTest::testDrupalGetHeader() is NOT working correctly, so
I don't think it'sIT IS my local setup.(EDIT. ARGH)
Comment #81
joachim CreditAttribution: joachim commented> I think we need to be clear what would be the effects of using dump() in the various contexts:
Yes, good point.
This should probably be documented too!
> 1) use of dump() in the SUT (i.e. in the runtime code) in Functional and FunctionalJavascript tests - where is the output going?
Into the web page that the SUT is serving, and so into the HTML output that the test generates.
It's basically similar to installing Devel module and using dpm()
> 2) the same for Kernel tests - both when run in isolation and not
In Kernel tests, dump() in both the test code and the SUT will product output on the command line where the test was run.
> 3) what would happen if a dump() is called in the context of a Unit or Build test?
In a Unit test, dump() will product output on the command line where the test was run.
> 4) use of dump() in the test code - where is the output going, both in isolation and not
In Unit and Kernel test code, dump() produces output on the command line where the test was run.
In Browser tests, it's currently causing a test failure. Fix for that coming!
Comment #83
joachim CreditAttribution: joachim commentedLatest changes:
- Made dump() working in the test code of browser tests
- Added documentation on how dump() works in kernel and browser tests
I think this is ready for review!
Comment #84
Spokjecspell didn't seem to like the latest patch
Comment #85
joachim CreditAttribution: joachim commentedThe cspell errors are all false positives.
How do we get Cspell to accept these?
> /var/www/html/core/tests/Drupal/KernelTests/KernelTestBase.php:69:21 - Unknown word (Dumper's)
> /var/www/html/core/tests/Drupal/Tests/BrowserTestBase.php:41:21 - Unknown word (Dumper's)
'Using Symfony VarDumper's dump()' should be fine. That's just a possessive on a proper noun.
datalen is a property of a PHP class. We can't change that.
PSFS is part of a PHP constant name.
Comment #86
joachim CreditAttribution: joachim commentedAh, I see how to do it. I've added those words to the cspell dictionary.
> If people really want the dump to stay permanently, they can opt out of phpunit's beStrictAboutTestsThatPrintOutput.
@jonathanshaw I assume you mean beStrictAboutOutputDuringTests? That's already enabled by default. The technique we use here to get dump() output to the command line bypasses PHPUnit's checks for output, because AFAIK if we don't, then the dump() output gets caught up in other stuff and it's not usable.
Comment #87
jonathanshawReally? We have no documentation or test coverage of this, and it's not clear to me why it would be true given that we haven't touched UnitTestCase.
Comment #88
joachim CreditAttribution: joachim commentedI just tried it, with a dump() both in the unit test method, and in a class being tested. dump() goes straight to the terminal with no errors or complaints.
This is just speculation, but do unit run without a child process? My vague impression is that the problem with dump() in tests is that it gets sucked up into the communication between the test process and the SUT process, but I don't actually understand how it works at all!
Should we add a test for dump() in unit tests?
Comment #89
jonathanshawI think so.
I'm not aware that Kernel use a child process either.
Comment #90
joachim CreditAttribution: joachim as a volunteer commented> I'm not aware that Kernel use a child process either.
There is definitely something very different between Unit and Kernel tests because *without* the work from this issue, dump() behaves very differently:
Unit test:
Kernel test:
Comment #91
joachim CreditAttribution: joachim as a volunteer commented> Should we add a test for dump() in unit tests?
I can't actually get a test for this to work!
Using the same technique as the Kernel test to capture the output doesn't work -- the dump() output is still visible, and the capture has nothing.
So I tried ob_start(). That doesn't work either!
I can't figure out which output stream is getting the dump() output in Unit tests.
Given that we are not changing anything to do with Unit tests in this patch, I would suggest that adding a test for dump() behaviour in unit tests could be left to a follow-up.
It would be really nice to get this in -- I think it makes a huge difference to DX for writing tests!
Comment #92
jonathanshawI can't RTBC until we've documented answers to #77 in the IS. Most of these points we've discussed, but some aren't clear to me.
Comment #93
joachim CreditAttribution: joachim as a volunteer commented@jonathanshaw updated the IS. Is this the sort of thing that's needed?
Comment #94
jonathanshawYes, but #77 asks about isolation
Comment #95
jonathanshawSorry, didn't mean to rtbc
Comment #96
joachim CreditAttribution: joachim commentedMy vague understanding was that ALL kernel tests run in isolation, and that's why we get the serialization crash if we try to use dump() in them at the moment.
Is there any documentation about how Drupal tests use PHPUnit process isolation functionality? I can't find very much about it.
Comment #97
mondrakeAh yes sorry, Kernel and Functional always run in isolation, my mistake. They extend from PHPUnit
TestCase
and setin the base classes, so that PHPUnit knows it will have to spawn a subprocess to execute the test.
The point of #77 (I should have been more clear) is that for Unit tests, they normally run in the same process where PHPUnit is executing, UNLESS you specify differently (setting the protected property or using the annotation
@runTestsInSeparateProcesses
. See for example Drupal\Tests\Core\Site\SettingsTest.So, using dump() in a Unit test that is set to run in isolation, what happens vs. a 'normal' Unit test?
Comment #98
joachim CreditAttribution: joachim commentedThanks for the explanation! (And I'm glad that matches with my vague understanding!!)
I'll investigate unit tests run in isolation.
Comment #99
mondrakeAfter this, we might go back to #2875038: AssertLegacyTrait::verbose() doesn't do anything and maybe just deprecate
AssertLegacyTrait::verbose()
and remove its usages.Comment #100
joachim CreditAttribution: joachim commented> So, using dump() in a Unit test that is set to run in isolation, what happens vs. a 'normal' Unit test?
The answer was that in an isolated unit test, dump() wasn't working -- it was crashing in the same was as in #90.
New commits fix that. As a byproduct, testing dump() in non-isolated unit tests works too!
Comment #101
jonathanshaw@joachim Could you add these notes on isolation to the IS?
Comment #102
joachim CreditAttribution: joachim commentedDone.
Comment #103
jonathanshawSeems like we're there.
Comment #104
daffie CreditAttribution: daffie commentedAdded a CR. See: https://www.drupal.org/node/3192283.
Comment #105
joachim CreditAttribution: joachim as a volunteer commentedThanks! That's great!
I've given it a little bit of a tweak.
Comment #106
mondrakeWorks nicely! Thanks for working on this, RTBC++
Comment #107
jungleNW for lowercasing the words in the dictionary, or/and adding a comment for
VarDumper::setHandler(TestVarDumper::class . '::cliHandler');
inUnitTestCase
explaining why it's not being called insetUpBeforeClass()
. Thanks!Comment #108
joachim CreditAttribution: joachim as a volunteer commented> lowercasing the words in the dictionary
Could you explain more please? I followed the instructions at https://www.drupal.org/node/3122084, which don't mention the case of words.
> explaining why it's not being called in setUpBeforeClass().
Good catch. It should be in setUpBeforeClass(), as it only needs to be done once for all tests in a class. Will fix.
Comment #109
jungle@joachim, see @alexpott's comment on the original issue here, please.
That CR needs updating, then.-- CR updated (edited)Comment #110
joachim CreditAttribution: joachim commentedFixed both of the issues in #107, and rebased the branch.
Comment #111
daffie CreditAttribution: daffie commented@joachim: You have merged the rebase and the fixes in one update. The problem with that as a reviewer there is now no interdiff.txt equavalent to easily review the changes that you made. I get an interdiff.txt with 251 files with +2056 lines added and -1164 lines removed.
@jungle asked for adding a comment, only one was not added.
Comment #112
jungle@daffie, probably, you can go to the commit tab on gitlab to review the latest 2 commits :), which is easier to review.
> Maybe, here worthy a comment, as we set the handler in setUpBeforeClass() in KernelTestBase.
The context was:
The handler in
KernelTestBase
was set in itssetUpBeforeClass()
, while the handler inUnitTestCase
was set in itssetup()
. The same handler was set in different methods, that's why I asked for a comment.In @joachim's latest commit, the set handler code for
UnitTestCase
was moved intosetUpBeforeClass()
, now, no more difference. So to me, a comment is unnecessary now.> Surprised this is a word in a dictionary :)
PSFS_FEED_ME
is a word in the dictionary still, just lowercased topsfs_feed_me
. I have not seen any valid word has_
. I believe there is something wrong, still. or needs a follow up to figure it out.Comment #113
jungleChecked locally,
psfs
is a new word, notPSFS_FEED_ME
/psfs_feed_me
. Please changepsfs_feed_me
in the dictionary topsfs
Comment #114
jungleTo correct, it's the Commits tab on the MR page, not commit tab. Sorry.
Comment #115
joachim CreditAttribution: joachim commented> @joachim: You have merged the rebase and the fixes in one update
Sorry... I figured it would be less confusing rather than adding commits that would immediately be obsolete. I didn't think of the need to see an interdiff.
> Please change psfs_feed_me in the dictionary to psfs
Done.
Thanks for your guidance on cspell configuration @jungle!
Comment #116
jungleIt looks ready to me unless the bot rejects it. Thanks!
Comment #117
mondrakeFiled #3193163: Deprecate AssertLegacyTrait::verbose and remove its usage
Comment #118
mondrakeNeeds a reroll.
Comment #119
mondrakeRerolled, patch context changes only.
Comment #120
alexpottAdded a code review to gitlab...
Comment #121
joachim CreditAttribution: joachim as a volunteer commentedAddressed all the points from @alexpott's review & rebased on 9.2.x.
Comment #122
mondrakeBack to RTBC then, thanks.
Comment #123
alexpottCommitted f7dd26a and pushed to 9.2.x. Thanks!
As per @mondrake's suggestion let's add a follow-up to deprecate common.inc's debug() function.
Fixed some things on commit.
dump() is a function not a method.
dump() function() is a lot of brackets.
Comment #125
andypostExisting issue for #123 follow-up #2002514-56: Deprecate debug(); remove references to _drupal_debug_message().
Comment #126
joachim CreditAttribution: joachim as a volunteer commented> dump() function() is a lot of brackets.
Oops!
I've published the draft CR.
Comment #128
neclimdulRan into the stream wrapping here triggering an edge case where symfony's deprecation handler triggers a php warning and leading to false positive failures.
Opened a pull with symfony.
https://github.com/symfony/symfony/pull/43149
Comment #129
quietone CreditAttribution: quietone commentedTwo followups were asked for in #74.
The first is #3193163: Deprecate AssertLegacyTrait::verbose and remove its usage
The second is #2002514: Deprecate debug(); remove references to _drupal_debug_message().
I am removing the tag.