In #2958429: Properly deprecate SafeMarkup::format() we finished conversion of the SafeMarkup utility component to the FormattableMarkup render component. However, the kernel test class was not renamed at the same time. We should rename this class so it refers to the thing that it covers, and also so that we can remove the last remnants of SafeMarkup in #3091447: Remove Utility component BC layers

CommentFileSizeAuthor
#13 interdiff.3098244.4-13.txt601 byteslongwave
#13 3098244-13.patch3.31 KBlongwave
#12 3098244-12.patch3.3 KBkrzysztof domański
#12 interdiff-11-12.txt761 byteskrzysztof domański
#11 Rename-SafeMarkupKernelTest-3098244-11.patch8.57 KBMadhura BK
#11 interdiff-3098244-9-11.txt541 bytesMadhura BK
#9 Rename-SafeMarkupKernelTest-3098244-9.patch8.58 KBMadhura BK
#9 interdiff-3098244-4-9.txt4.34 KBMadhura BK
#4 interdiff.3098244.2-4.txt573 byteslongwave
#4 3098244-4.drupal.Rename-SafeMarkupKernelTest-to-FormattableMarkupKernelTest.patch3.28 KBlongwave
#2 3098244.patch3.11 KBlongwave

Comments

longwave created an issue. See original summary.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Active » Needs review
StatusFileSize
new 3098244.patch3.11 KB

Status: Needs review » Needs work

The last submitted patch, 2: 3098244.patch, failed testing. View results

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review
StatusFileSize
new 3098244-4.drupal.Rename-SafeMarkupKernelTest-to-FormattableMarkupKernelTest.patch3.28 KB
new interdiff.3098244.2-4.txt573 bytes

It would help if I used the correct namespace.

andypost’s picture

andypost
he/him
Primary language Russian
commented

It looks good, but when we rename test classes we should left a copy of existing one and remove it in d9?!

krzysztof domański’s picture

krzysztof domański
Location Poland
commented
Status: Needs review » Needs work

1. @group Render instead of @group Utility.

  * @group Utility
 */

2. We have several tests in the core/tests/Drupal/KernelTests/Core/Render directory. Can we move this test there?

--- a/core/tests/Drupal/KernelTests/Component/Render/FormattableMarkupKernelTest.php
+++ b/core/tests/Drupal/KernelTests/Core/Render/FormattableMarkupKernelTest.php

3. Re #5: Only one unsupported contrib module uses this test. Browser development module is not used. I think we can keep the old (deprecated) test, but IMO it won't be bad if we remove it.

http://grep.xnddx.ru/search?text=SafeMarkupKernelTest&filename=

Madhura BK’s picture

Madhura BK commented
Assigned: Unassigned » Madhura BK
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Re: #6.3 that is an unused use statement anyway, so I think we can say the old class is safe to remove.

Madhura BK’s picture

Madhura BK commented
Assigned: Madhura BK » Unassigned
Status: Needs work » Needs review
StatusFileSize
new interdiff-3098244-4-9.txt4.34 KB
new Rename-SafeMarkupKernelTest-3098244-9.patch8.58 KB

Have implemented the changes suggested in #6

krzysztof domański’s picture

krzysztof domański
Location Poland
commented
Status: Needs review » Needs work

@Madhura BK Thanks! We need to change the namespace after moving to another directory.

-namespace Drupal\KernelTests\Component\Render;
+namespace Drupal\KernelTests\Core\Render;

Madhura BK’s picture

Madhura BK commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-3098244-9-11.txt541 bytes
new Rename-SafeMarkupKernelTest-3098244-11.patch8.57 KB

Oh, I had forgotten to change the namespace.Have fixed it in this patch.Thanks Krzysztof Domański.

krzysztof domański’s picture

krzysztof domański
Location Poland
commented
StatusFileSize
new interdiff-11-12.txt761 bytes
new 3098244-12.patch3.3 KB

I realised that FormattableMarkup class is inside core/lib/Drupal/Component/Render so test should be inside core/tests/Drupal/KernelTests/Component/Render.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
StatusFileSize
new 3098244-13.patch3.31 KB
new interdiff.3098244.4-13.txt601 bytes

There is also a misaligned comment marker that we might as well fix while we are here. Interdiff is from #4.

krzysztof domański’s picture

krzysztof domański
Location Poland
commented
Status: Needs review » Reviewed & tested by the community

All feedback (#6, #8) was addressed.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed fd224bc774 to 9.0.x and 5d64d72c03 to 8.9.x and 82aea19c5d to 8.8.x. Thanks!

Backported to 8.8.x since this is a test-only change.

@andypost tests are not API - (test traits can be though).

  • alexpott committed fd224bc on 9.0.x 
    Issue #3098244 by longwave, Madhura BK, Krzysztof Domański: Rename...

  • alexpott committed 5d64d72 on 8.9.x 
    Issue #3098244 by longwave, Madhura BK, Krzysztof Domański: Rename...

  • alexpott committed 82aea19 on 8.8.x 
    Issue #3098244 by longwave, Madhura BK, Krzysztof Domański: Rename...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.