The changes to Drupal\KernelTests\KernelTest->render() in #2564547: Remove calls to drupal_process_attached were incorrect, because both the input and output variables used in the test are null. This will presumably result in this test always passing.
Use real values.
Write the patch
None - bug fix in tests
None
None
| Issue category | Bug because some tests are not failing when they should |
|---|---|
| Issue priority | Major because other patches might be passing when they should fail. |
| Unfrozen changes | Unfrozen because it only changes tests. |
| Disruption | Not disruptive |
| Comment | File | Size | Author |
|---|---|---|---|
| #23 | 2568511_23.patch | 7.96 KB | wim leers |
| #11 | 2568511_11.patch | 8.43 KB | mile23 |
| #8 | interdiff.txt | 1.36 KB | mile23 |
| #8 | 2568511_8.patch | 5.4 KB | mile23 |
| #5 | interdiff.txt | 3.47 KB | mile23 |
Comments
Comment #2
ianthomas_ukAlso brought the simpletest version inline with the core/tests version, which was missed in the previous issue.
Comment #5
mile23Ugh.
Nice catch.
Here are some fixes.
We do a check for
html_response_attachment_placeholdersinHtmlResponseAttachmentsProcessorand add a check inTableTest.Will need to reroll #2477223: Refactor _drupal_add_html_head, drupal_get_html_head, _drupal_add_html_head_link into the attachments processor, remove from common.inc. with this change to
HtmlResponseAttachmentsProcessor.Comment #8
mile23And fixing
TableTest...Comment #9
ianthomas_ukI figure @Mile23 is happy with the changes I made to the two KernelTest::render() methods.
The other changes are his and are sensible. HtmlResponseAttachmentsProcessor is just an extra if (the noise is just because of indenting) and the new TableTest is nicer than the old, as it tests the rendered output rather than the intermediate data structure.
Comment #10
mile23This will need a reroll after #2477223: Refactor _drupal_add_html_head, drupal_get_html_head, _drupal_add_html_head_link into the attachments processor, remove from common.inc.
Comment #11
mile23And here's your re-roll.
The patch around
HtmlResponseAttachmentsProcessor::processAttachment()looks hairy but all it does is bracket the processing by checking whether$attached['html_response_attachment_placeholders']exists. git apparently can't say 'just indent all this code by 2 spaces.'Comment #12
mile23Comment #13
ianthomas_ukReroll is good. Git might not be able to just indent a block of code, but source tree can ignore white space ;)
Comment #15
mile23Pretty sure the fate of this patch might be tied up in #2571427: HtmlResponseAttachmentsProcessor tests misplaced during reroll
Not sure if it's postponed or what.
Comment #18
mile23Failing test is
MigrateUserProfileEntityFormDisplayTest, which passes locally with and without the patch in #11.I'm calling this an unrelated fail.
Comment #19
fabianx commentedLooks good to me, looks like a bug fix to me, too.
Comment #20
mile23Comment #21
wim leersAt first sight, the changes in
\Drupal\Core\Render\HtmlResponseAttachmentsProcessor::processAttachments()look excessive.But on second sight it's just wrapping that logic in an if-test. It's totally harmless and in fact makes the logic a bit easier to follow. I support this.
Comment #22
alexpottNeeds a reroll.
Comment #23
wim leers#2555069: Remove invocation of hook_html_head_alter() conflicted with this. Smaller patch, because fewer LoC remain to be wrapped in that if-statement, hence fewer changes.
Comment #24
alexpottDiscussed with @Wim Leers in IRC - we agreed that this change makes sense is very low risk and not disruptive. Committed a47dd7a and pushed to 8.0.x. Thanks!
Comment #27
dawehnermeh