FileFieldTestBase::assertFileEntryNotExists() not properly typehinted, missing default $message parameter

The Needs Review Queue Bot → tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

liam morland → made their first commit to this issue’s fork.

If the value is NULL, the automatic message will result in this error:

Argument #2 ($message) must be of type string, Drupal\Component\Render\FormattableMarkup

Can issue summary be updated to use standard template.

Pretty sure javascript is a random failure.

Shouldn't the default be '' and don't need to typecast?

Other ones have NULL as the default. The cast is needed anyway because of the error I mentioned in #16.

Gotcha,

Change seems small then so don't mind marking.

I'm setting back to RTBC since there is just one change (setting default value to param) in the test file now.
MR is updated with latest 11.x
Tests are green as well.

The method is missing its parameter documentation, which does not help the situation. Given that we're changing the allowed values of the assertion method, I think it's also in scope to add parameter docs.

Since the current version is only broadening the allowed values of the method, it's pretty BC. That said, since we're talking of typehints, I think we can also expand the scope to add signature typehints to the assertion, which is an allowable BC break for test-only code. I checked contrib usages and overrides of this assertion, and there's only one contrib module in a non-D7 test: MinisiteTestBase.

Thanks everyone!

Sorry, I should have provided the reference to the BC policy regarding test code → . In this case, since it's a method on a base class, it's borderline, but as a release manager I have discretion to allow internal BC breaks in certain cases like this one. :) Thanks!

Thanks for clarifying and updating the issue.
I've made necessary changes to the assertion call.
MR is green now.

I have rebased this. Is there a reason this does not also add type declarations to the other methods in that file?

@liam morland That could be done elsewhere -- and there are meta issues to address missing typehints in core generally -- but it is out of scope for this issue, which is just about the one specific method. Thanks!

Feedback appears to be addressed

Committed to 11.x despite the BC break, based on my assessment in #27. I also backported it to 11.2.x in accordance with 📌 [policy, no patch] Update allowed changes to current practice for backport of Test API changes Active .

Thanks everyone!

xjm → closed merge request !9344