Skip to content

Conversation

@dpi
Copy link
Owner

@dpi dpi commented Dec 14, 2017

if ($form_state->getValue('skip_queue')) {
$skip_queue = $form_state->getValue('skip_queue');
$verbose = $form_state->getValue('verbose');
if ($verbose && $skip_queue) {

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?

Copy link
Owner Author

@dpi dpi Dec 16, 2017

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',

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.

Copy link
Owner Author

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]);

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

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);

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.

Copy link
Owner Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants