Error message not showing when no source object selected

+++ b/src/Form/SourceOverviewForm.php
@@ -153,10 +153,15 @@ class SourceOverviewForm extends FormBase {
+    $items = &$form['items']['#value'];
+    $issearch = FALSE;
+    if ($_POST['op'] == t('Search')) {
+      $issearch = TRUE;
+    }
+    if (empty($items) && !$issearch) {

Why do you need to get the value from $form? And you shouldn't use $_POST, again, use $form_state->getValue().

+++ b/src/Form/SourceOverviewForm.php
@@ -153,10 +153,15 @@ class SourceOverviewForm extends FormBase {
+    $items = $form['items']['#value'];

Did you try array_filter() on getValue() ?

+++ b/src/Form/SourceOverviewForm.php
@@ -153,10 +153,15 @@ class SourceOverviewForm extends FormBase {
+    $issearch = FALSE;
...
+      $issearch = TRUE;

Based on the coding standards, I would separate these two words in the variable with an underscore. It is much easier to read that way.

+++ b/modules/demo/src/Tests/TMGMTDemoTest.php
@@ -50,6 +50,9 @@ class TMGMTDemoTest extends TMGMTTestBase {
+    $this->assertUniqueText(t("You didn't select any source objects"));

+++ b/src/Form/SourceOverviewForm.php
@@ -153,10 +153,15 @@ class SourceOverviewForm extends FormBase {
+      $form_state->setError($form, t("You didn't select any source objects"));

Sorry, you're missing a dot at the end of the sentence. :)

1. +++ b/modules/demo/src/Tests/TMGMTDemoTest.php
   @@ -50,6 +50,9 @@ class TMGMTDemoTest extends TMGMTTestBase {
   +    $this->assertUniqueText(t("You didn't select any source objects."));
   
   +++ b/src/Form/SourceOverviewForm.php
   @@ -153,10 +153,15 @@ class SourceOverviewForm extends FormBase {
   +      $form_state->setError($form, t("You didn't select any source objects."));
   

   Comment #10 from #2511822: False past broken info on status report :)

2. +++ b/src/Form/SourceOverviewForm.php
   @@ -153,10 +153,15 @@ class SourceOverviewForm extends FormBase {
   +    if ($form_state->getValue('op') == t('Search')) {
   

   I guess you can use $form['search_wrapper']['search_submit']['#value'] == $form_state->getTriggeringElement()['#value'] so we don't need to translate new Search again.

   Btw, there is a Cancel button which displays "You didn't select any source objects" when triggered. Is this Cancel about translation or filters? If it is like Search, then I think we should do the same for it.

3. +++ b/src/Form/SourceOverviewForm.php
   @@ -153,10 +153,15 @@ class SourceOverviewForm extends FormBase {
   +    if (empty($items) && !$is_search) {
   

   You can use !$items instead of empty($items).

+++ b/src/Form/SourceOverviewForm.php
@@ -153,10 +153,21 @@ class SourceOverviewForm extends FormBase {
+    if (array_key_exists('search_cancel', $form['search_wrapper'])) {

Is always true?

Remove that, also looks nicer than two nested if statements.

+++ b/src/Form/SourceOverviewForm.php
@@ -153,10 +153,19 @@ class SourceOverviewForm extends FormBase {
+    if ($form['search_wrapper']['search_cancel']['#value'] == $form_state->getTriggeringElement()['#value']) {
+      $is_cancel = TRUE;
+    }

I think you have to check if $form['search_wrapper']['search_cancel'] exists before calling the child element.

+++ b/src/Form/SourceOverviewForm.php
@@ -153,10 +153,19 @@ class SourceOverviewForm extends FormBase {
+    $items = array_filter($form_state->getValue('items'));
+    $is_search = FALSE;
+    $is_cancel = FALSE;
+    if ($form['search_wrapper']['search_submit']['#value'] == $form_state->getTriggeringElement()['#value']) {
+      $is_search = TRUE;
+    }
+    if ($form['search_wrapper']['search_cancel']['#value'] == $form_state->getTriggeringElement()['#value']) {
+      $is_cancel = TRUE;
+    }
+    if (!$items && !$is_search && !$is_cancel) {
+      $form_state->setError($form, t("You didn't select any source objects."));
+    }
+    $plugin = $form_state->get('plugin');
     $item_type = $form_state->get('item_type');

Not really happy about this.

We fixed locale now but this still means we have hardcoded assumptions that those form elements exist.. there might be a source that doesn't even have a search.

We could make it conditional I guess, but what about turning this around...

We could move it to overviewFormValidate() in the plugin Ui base class.

Or, what if we add a validateItemsSelected() method to the baes class and explicitly add it to the buttons that actually want it? Combined with unifying that part into the base class, that would make it standardized but still overridable.

1. +++ b/modules/demo/src/Tests/TMGMTDemoTest.php
   @@ -50,6 +50,15 @@ class TMGMTDemoTest extends TMGMTTestBase {
        $this->assertText(t('2 jobs need to be checked out.'));
   +    // Check to see if no items are selected and the error message pops up.
   +    $this->drupalPostForm('admin/tmgmt/sources', array(), t('Request translation'));
   +    $this->assertUniqueText(t("You didn't select any source objects."));
   +    $this->drupalPostForm('admin/tmgmt/sources', array(), t('Search'));
   +    $this->assertNoText(t("You didn't select any source objects."));
   +    $this->drupalPostForm('admin/tmgmt/sources', array(), t('Cancel'));
   +    $this->assertNoText(t("You didn't select any source objects."));
   +    $this->drupalPostForm('admin/tmgmt/sources', array(), t('Add to cart'));
   +    $this->assertUniqueText(t("No job items were selected."));
   

   How is that we have two different messages here?

2. +++ b/src/Form/SourceOverviewForm.php
   @@ -135,6 +135,7 @@ class SourceOverviewForm extends FormBase {
        $form['actions']['submit'] = array(
          '#type' => 'submit',
   +      '#validate' => array('::validateItemsSelected'),
          '#value' => t('Request translation'),
        );
   

   The same validate needs to be added to the add to cart form.

3. +++ b/src/Form/SourceOverviewForm.php
   @@ -212,5 +210,23 @@ class SourceOverviewForm extends FormBase {
   +    $items = array_filter($form_state->getValue('items'));
   +    $is_search = FALSE;
   +    $is_cancel = FALSE;
   +    if ($form['search_wrapper']['search_submit']['#value'] == $form_state->getTriggeringElement()['#value']) {
   +      $is_search = TRUE;
   +    }
   +    if ($form['search_wrapper']['search_cancel']['#value'] == $form_state->getTriggeringElement()['#value']) {
   +      $is_cancel = TRUE;
   +    }
   +    if (!$items && !$is_search && !$is_cancel) {
   +      $form_state->setError($form, t("You didn't select any source objects."));
   +    }
   +  }
   

   Still not cleaned up.

4. +++ b/src/SourcePluginUiBase.php
   @@ -60,6 +60,10 @@ class SourcePluginUiBase extends PluginBase implements SourcePluginUiInterface {
        // Nothing to do here by default.
   +    $items = array_filter($form_state->getValue('items'));
   +    if (!$items && ($form_state->getTriggeringElement('#value') == t('Request translation') || ($form_state->getTriggeringElement('#value') == t('Cancel')))) {
   +      $form_state->setError($form, t("You didn't select any source objects."));
   +    }
   

   This is not needed?

1. +++ b/src/Form/SourceOverviewForm.php
   @@ -153,10 +155,7 @@ class SourceOverviewForm extends FormBase {
   -    /*if (!$form_state->getValue('values')) {
   -      $form_state->setErrorByName('items', t("You didn't select any source objects"));
   -    }*/
   -    $plugin =  $form_state->get('plugin');
   +    $plugin = $form_state->get('plugin');
        $item_type = $form_state->get('item_type');
        // Execute the validation method on the source plugin controller.
        $source_ui = $this->sourceManager->createUIInstance($plugin);
        $source_ui->overviewFormValidate($form, $form_state, $item_type);
   

   I would rewrite this as:

       $source_ui = $this->sourceManager->createUIInstance($form_state->get('plugin'));
       $source_ui->overviewFormValidate($form, $form_state, $form_state->get('item_type'));
   

   Same for submitForm()

2. +++ b/src/Form/SourceOverviewForm.php
   @@ -212,5 +211,15 @@ class SourceOverviewForm extends FormBase {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function validateItemsSelected(array $form, FormStateInterface $form_state) {
   

   Method documentation seems wrong?

3. +++ b/src/Form/SourceOverviewForm.php
   @@ -212,5 +211,15 @@ class SourceOverviewForm extends FormBase {
   +  public function validateItemsSelected(array $form, FormStateInterface $form_state) {
   +    $items = array_filter($form_state->getValue('items'));
   +    if (!$items) {
   +      $form_state->setError($form, t("You didn't select any source items."));
   +    }
   +  }
   
   +++ b/tmgmt.module
   @@ -1066,7 +1066,7 @@ function tmgmt_source_add_to_cart_submit(array $form, FormStateInterface $form_s
    function tmgmt_cart_source_overview_validate(array $form, FormStateInterface $form_state) {
      $items = array_filter($form_state->getValue('items'));
      if (empty($items)) {
   -    $form_state->setError($form, t('No job items were selected.'));
   +    $form_state->setError($form, t("You didn't select any source items."));
      }
    }
   

   Are the both methods (with same code) needed? If yes, can we call tmgmt_cart_source_overview_validate() from validateItemsSelected() and avoid code duplication?

+++ b/modules/demo/src/Tests/TMGMTDemoTest.php
@@ -50,6 +50,15 @@ class TMGMTDemoTest extends TMGMTTestBase {
+    $this->drupalPostForm('admin/tmgmt/sources', array(), t('Search'));
...
+    $this->drupalPostForm('admin/tmgmt/sources', array(), t('Cancel'));
...
+    $this->drupalPostForm('admin/tmgmt/sources', array(), t('Add to cart'));

$this->drupalPostForm(NULL, [], t('Search')); is shorter and consistent regarding the array syntax. Leave the path in the first drupalPostForm, replace the rest with NULL (since we already are on that page), switch the array()'s to [].

+++ b/modules/demo/src/Tests/TMGMTDemoTest.php
@@ -50,6 +50,15 @@ class TMGMTDemoTest extends TMGMTTestBase {
+    $this->drupalPostForm('admin/tmgmt/sources', array(), t('Request translation'));

another array()

The interdiff is something, but not an interdiff :P.

Looks good now.

1. +++ b/src/Form/SourceOverviewForm.php
   @@ -130,11 +130,13 @@ class SourceOverviewForm extends FormBase {
          '#title' => t('Operations'),
          '#open' => TRUE,
   -      '#attributes' => array('class' => array('tmgmt-source-operations-wrapper'))
   +      '#attributes' => array('class' => array('tmgmt-source-operations-wrapper')),
   +      '#validate' => array('::validateItemsSelected'),
   

   A #validate on a details element doesn't do anything?

2. +++ b/src/Form/SourceOverviewForm.php
   @@ -212,5 +211,17 @@ class SourceOverviewForm extends FormBase {
   +   */
   +  public function validateItemsSelected(array $form, FormStateInterface $form_state) {
   +    tmgmt_cart_source_overview_validate($form, $form_state);
   +  }
   

   This is a bit weird but OK, can live with that.

+++ b/src/Form/SourceOverviewForm.php
@@ -130,11 +130,12 @@ class SourceOverviewForm extends FormBase {
       '#open' => TRUE,
-      '#attributes' => array('class' => array('tmgmt-source-operations-wrapper'))
+      '#attributes' => array('class' => array('tmgmt-source-operations-wrapper')),
     );

I guess then this change is not needed.

Seems fine.

Looking good now.

Ok, committed, thanks.