Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
See parent(s).
This issue is for complex conversions of FormattableMarkup into regular double-quoted strings with simple function calls or new variables used in the arguments embedded directly inside the quoted string. Occasional use of sprintf may also be considered.
The scope here is the core Functional tests.
Steps to reproduce
Proposed resolution
Convert all test assertion methods in functional tests from using FormattableMarkup that are not covered in #3404273: Fix strict type errors: Convert FormattableMarkup to strings (simple replacement) in core Functional tests.
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | 3412464-nr-bot.txt | 9.51 KB | needs-review-queue-bot |
Issue fork drupal-3412464
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:
changes
Comments
Comment #3
mstrelan CreditAttribution: mstrelan at PreviousNext commentedThink this should cover everything not in #3404273: Fix strict type errors: Convert FormattableMarkup to strings (simple replacement) in core Functional tests.
To be honest I don't really know when to use string concatenation, string interpolation or sprintf. Most of the time it doesn't seem worth it to add new variables. I've done what seems reasonable to me. The goal is to add the strict_types declaration and I don't think there's much point debating how to format these.
Comment #4
smustgrave CreditAttribution: smustgrave commentedFor the goal of getting strict_type added these seem fine.
I also don't know when to use what, and not sure it should be enforced till there is a rule that can be followed. Till then seems to be each person's personal preference, just my observation.
Comment #5
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #6
mstrelan CreditAttribution: mstrelan at PreviousNext commentedRebased
Comment #7
smustgrave CreditAttribution: smustgrave commentedDoesn't seem to break anything and still appears to give same strings.
Still fuzzy on when sprintf, formattableMarkup, or double-quotes strings should be used.
Comment #8
quietone CreditAttribution: quietone at PreviousNext commentedI read through the MR and didn't have any questions. I randomly selected a few to test by forcing the error so the message would display. And like @smustgrave, I do see that the changes sometimes uses sprintf and sometimes do not. But that is also personal style and the best fit for the use case. I think this is good to commit and will assign to myself to commit tomorrow.
Comment #11
quietone CreditAttribution: quietone at PreviousNext commentedReally nice to see FormattableMarkup removed here.
I wanted to commit to 10.2.x to keep the tests in sync but this no longer applies to 10.2.x.
Committed 503192f and pushed to 11.x. Thanks!
Comment #13
mstrelan CreditAttribution: mstrelan at PreviousNext commentedCreated a new branch from 10.2.x and cherry-picked the same commits. Setting RTBC as it's the same as was committed to 11.x, but let me know if this needs to go to NR first.
Comment #15
quietone CreditAttribution: quietone at PreviousNext commented@mstrelan, thank you.
Committed 362a5f6 and pushed to 10.2.x to keep tests in sync. Thanks!