-
Notifications
You must be signed in to change notification settings - Fork 0
Sms devel result reports output #70
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 8.x-1.x
Are you sure you want to change the base?
Conversation
| if ($form_state->getValue('skip_queue')) { | ||
| $skip_queue = $form_state->getValue('skip_queue'); | ||
| $verbose = $form_state->getValue('verbose'); | ||
| if ($verbose && $skip_queue) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean we can't queue verbose messages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You cant verbose queue messages, because they arnt handled in the same request as devel. There is no result yet.
| '#type' => 'table', | ||
| '#caption' => [ | ||
| 'heading' => [ | ||
| '#type' => 'inline_template', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be '#type' => 'markup' since you're not using any twig placeholders. Inline template has a bit of twig overhead to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Results" should be a translatable/placeholder.
Overhead isnt really a problem given its renderred once.
|
|
||
| foreach ($results as $i => $result) { | ||
| $row = []; | ||
| $row[]['#plain_text'] = $this->t("#@number", ['@number' => $i]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't need to be translatable, we could use new FormattableMarkup('#@number', ['@number' => $i]); here.
| * Tests the message form. | ||
| * | ||
| * @group SMS Framework | ||
| * @group legacy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will introduce one test fail on D8.5
| $edit['skip_queue'] = TRUE; | ||
|
|
||
| $this->drupalPostForm(Url::fromRoute('sms_devel.message'), $edit, t('Send')); | ||
| $this->assertResponse(200); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it's ok to remove deprecated code usages even though it introduces scope creep.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can commit the deprecation commit separately instead of squashing the whole thing.
https://www.drupal.org/project/smsframework/issues/2856048