Fix strict type errors in AssertContentTrait

Thought about asking if we could add the declare for this one line, but guess since it's a trait it would cause failures where used. So think this looks good.

Think adding the declaration is still blocked on #3303206: Define a standard for adding declare(strict_types=1) → anyway

Agreed this is RTBC, and that we shouldn't add the declaration itself.

Yet, I wonder about the docs changes here in relation to 📌 Fix usages of AssertContentTrait and stop recommending FormattableMarkup Active . If we always want these to be strings, shouldn't we keep documenting them as such? Do we want to relax all these per the MR? Should we be more rigorous about wanting to be passed string and not MarkupInterface?

Before we ask committers to look at this, let's decide if these two issues should be merged, or what.

My main concern is if contrib is using this trait. We could of course deprecate using FormattableMarkup.

If it could cause issues for contrib could we do the deprecation dance? Did a basic search https://git.drupalcode.org/search?group_id=2&page=7&scope=blobs&search=A... and a few modules are using it. Guess they may be using it right but no way to really know?

In its current state it won't cause issues, if we restrict it to string only then it might. I actually think keeping the union type for anything other than $message is helpful here.

So just for clarity, "needs work" here is for a decision on the deprecation. Since we're only changing the annotation here I think deprecating can be done later, and we can add an actual type hint then too.

Reverted all docs changes, I think they should be handled in 📌 Fix usages of AssertContentTrait and stop recommending FormattableMarkup Active . So now we're essentially just casting $message to a string in cases where a MarkupInterface may have been passed.

Seems like a simple enough change.

Some questions about consistency on the MR. But can't we just change the method signatures here instead of casting?

protected function setRawContent(string $content) {

FormattableMarkup will be cast to string here, until we add declare(strict_types=1); to the file, at which point errors will occur.

But we could do that now anyway, and if over in 📌 Fix usages of AssertContentTrait and stop recommending FormattableMarkup Active we decide to start issuing deprecations, we could switch to union types?

protected function setRawContent(string|FormattableMarkup $content) {
  if (is_object($content)) {
    @trigger_error(...)

I'm keen to do whatever will get 📌 [PP-2] Add declare(strict_types=1) to all Kernel tests Postponed in the fastest, to avoid having to reroll related tickets. Perhaps we add this file to the exclude patterns while we sort it out properly. Alternatively the changes in this MR do the bare minimum to get it green. Happy to be guided here.

FWIW I tried to exclude this file from strict types in !MR 6826 and it resulted in only one error, from WhoIsOnlineBlockTest calling strpos on $this->getRawContent() which returns a MarkupInterface instead of string. So we could probably cast that one instance and keep this excluded while we work out the correct path forward.

If we replace FormattableMarkup throughout then the doc comments would have to be updated but that would mean a bunch of tests would be technically incorrect right?

Following up on #16?

Aiming to work on this tomorrow at Drupal South Sydney code sprint.

Sounds like there is more work to be done, (if I'm wrong apologize and please put back into review)

I have read the IS and comments. The title here is to fix strict type errors and the changes made here do that. Without these changes there are tests that fail. I agree with longwave that the other instances should be fixed but let's put that in the existing followup. It can be done there by expanding the scope in the separate issue, which will include adjusting the documentation.

Sounds like a plan @quietone moving to NW for the other threads though.

@smustgrave I thought @quietone was suggesting the threads in the MR should move to a follow up? But given the current state of strict types in tests I think we should add declare(strict_types=1) in this issue and remove <exclude-pattern>./tests/Drupal/KernelTests/AssertContentTrait.php</exclude-pattern> from phpcs.xml.dist. So leaving NW for that.

Maybe I misread I thought the one thread about other instances was the follow up but other threads were good

Made the phpcs changes, let's see if it's green. IMHO if it's green then we've done everything in scope and any other refactoring can go to 📌 Fix usages of AssertContentTrait and stop recommending FormattableMarkup Active .

Looks like it's failing.

Trying to keep up, sorry for the delay, looking at what's currently in the MR I believe everything is good and scope of this ticket. With the follow up filed. Going to mark but if I missed something that's my bad.

OK, for the sake of this one file and to close this off let's just get this in and improve the typing in followups.

Backported to 10.4.x to keep things in sync over there. Not sure this is worth the effort of backporting to release branches and there is the vague possibility we end up breaking someone's test somehow with this.

Committed 99e7668 and pushed to 11.x. Thanks!
Committed 8964a2d and pushed to 10.4.x. Thanks!

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