Sub-issue for meta issue #1880976: [meta] Port examples (including submodules) to D9.4+

Problem/Motivation

D8 all the things!

Proposed resolution

Start with D7 version and figure out how to port ;)

CommentFileSizeAuthor
#59 interdiff-58.txt288 bytesnavneet0693
#59 2102703-58.patch21.21 KBnavneet0693
#56 interdiff-56.txt9.78 KBTorenware
#56 2102703-56.patch21.21 KBTorenware
#54 interdiff_46.txt1.68 KBMile23
#54 2102703_54.patch19.91 KBMile23
#46 port_queue_example-2102703-45.patch19.62 KBnavneet0693
#44 port_queue_example-2102703-44.patch19.72 KBnavneet0693
#39 interdiff-39.txt642 bytesTorenware
#39 port_queue_example-2102703-39.patch19.66 KBTorenware
#36 image6.png31.58 KBShreya Shetty
#33 QueueExampleTestCase.png54.04 KBnavneet0693
#30 port_queue_example-2102703-30.patch19.68 KBnavneet0693
#30 interdiff.txt5.21 KBnavneet0693
#24 port_queue_example-2102703-24.patch19.3 KBnavneet0693
#24 interdiff.txt4.7 KBnavneet0693
#16 interdiff.txt418 bytesm4olivei
#16 2102703_16.patch17.69 KBm4olivei
#14 2102703-14-different-implementation.patch16.54 KBlegovaer
#11 interdiff.txt11.61 KBm4olivei
#11 2102703_11.patch17.69 KBm4olivei
#10 interdiff.txt12.29 KBm4olivei
#9 2102703_9.patch17.01 KBm4olivei
#6 interdiff-do-not-test.diff952 bytestsphethean
#6 2102703-6-port_queue_examples_d8.patch15.96 KBtsphethean
#4 2102703-4-port_queue_examples_d8.patch15.96 KBtsphethean
#2 2102703-2-port_queue_examples_d8.patch16.43 KBtsphethean
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tsphethean’s picture

Assigned: Unassigned » tsphethean
Issue summary: View changes
tsphethean’s picture

Status: Active » Needs review
FileSize
16.43 KB

Direct port from D7 version, with simpletests passing locally

sdelbosc’s picture

Code looks good to me. Only few remarks:

  • Indentation seems wrong in QueueExampleForm::buildForm().
  • Is the implementation of QueueExampleForm::validateForm() really mandatory?
  • I think QueueExampleTestCase::web_user can be removed.
  • I think QueueExampleTestCase::setUp() can be removed in D8.
tsphethean’s picture

Thanks for the review.

Indentation seems wrong in QueueExampleForm::buildForm().

Good spot - thanks. Fixed

Is the implementation of QueueExampleForm::validateForm() really mandatory?

It was, because I implemented FormInterface instead of extending FormBase - now correctly extending FormBase and removed validateForm()

I think QueueExampleTestCase::web_user can be removed.
I think QueueExampleTestCase::setUp() can be removed in D8.

Good spot - thanks. Fixed.

Revised patch attached.

Mile23’s picture

Status: Needs review » Needs work
  1. +++ b/queue_example/lib/Drupal/queue_example/Tests/QueueExampleTestCase.php
    @@ -0,0 +1,73 @@
    +/**
    + * cron_example test class
    + */
    

    Woops, wrong cut and paste. :-) This is probably in the D7 version, too, I bet.

  2. +++ b/queue_example/lib/Drupal/queue_example/Tests/QueueExampleTestCase.php
    @@ -0,0 +1,73 @@
    +  /**
    +   * Login user, create an example node, and test blog functionality through the admin and user interfaces.
    +   */
    

    80 char wrap, please.

tsphethean’s picture

Status: Needs work » Needs review
FileSize
15.96 KB
952 bytes

@Mile23 thanks for the review. I've fixed the two comments in the new patch.

Mile23’s picture

Mile23’s picture

Status: Needs review » Needs work

Thanks, tsphethean.

+++ b/queue_example/lib/Drupal/queue_example/Forms/QueueExampleForm.php
@@ -0,0 +1,263 @@
+    $queue_name = !empty($form_state['values']['queue_name']) ? $form_state['values']['queue_name'] : 'queue_example_first_queue';
...
+    // $form['#attached']['css'] = array(drupal_get_path('module', 'queue_example') . '/queue_example.css');
...
+      '#options' => MapArray::copyValuesToKeys(array('queue_example_first_queue', 'queue_example_second_queue')),
...
+      '#default_value' => t('item @counter', array('@counter' => $form_state['storage']['insert_counter'])),
...
+      '#options' => array(0 => t('none'), 5 => t('5 seconds'), 60 => t('60 seconds')),
...
+      '#default_value' => !empty($form_state['values']['claim_time']) ? $form_state['values']['claim_time'] : 5,
...
+   * It is not recommended to access the database directly, and this is only here
+   * so that the user interface can give a good idea of what's going on in the
+   * queue.
...
+    drupal_set_message(t('Queued your string (@string_to_add). There are now @count items in the queue.', array('@count' => $count, '@string_to_add' => $form_state['values']['string_to_add'])));
+    $form_state['rebuild'] = TRUE;  // Allows us to keep information in $form_state.
...
+    // Unsetting the string_to_add allows us to set the incremented default value
...
+      drupal_set_message(t('There are now @count items in the queue.', array('@count' => $count)));
...
+    drupal_set_message(t('Deleted the @queue_name queue and all items in it', array('@queue_name' => $form_state['values']['queue_name'])));

+++ b/queue_example/lib/Drupal/queue_example/Tests/QueueExampleTestCase.php
@@ -0,0 +1,74 @@
+      $edit = array('queue_name' => 'queue_example_first_queue', 'string_to_add' => "boogie$i");
+      $this->drupalPostForm('queue_example/insert_remove', $edit, t('Insert into queue'));
+      $this->assertText(t('There are now @number items in the queue', array('@number' => $i)));
...
+      $edit = array('queue_name' => 'queue_example_first_queue', 'claim_time' => 0);
+      $this->drupalPostForm(NULL, $edit, t('Claim the next item from the queue'));
+      $this->assertPattern(t('%Claimed item id=.*string=@string for 0 seconds.%', array('@string' => "boogie$i")));
...
+    $edit = array('queue_name' => 'queue_example_first_queue', 'claim_time' => 0);
...
+      $edit = array('queue_name' => 'queue_example_first_queue', 'claim_time' => 0);
+      $this->drupalPostForm(NULL, $edit, t('Claim the next item and delete it'));
...
+    $edit = array('queue_name' => 'queue_example_first_queue', 'claim_time' => 0);

+++ b/queue_example/queue_example.module
@@ -0,0 +1,68 @@
+      $item['expire'] = t("Claimed: expires %expire", array('%expire' => date('r', $item['expire'])));
...
+    $header = array(t('Item ID'), t('Claimed/Expiration'), t('Created'), t('Content/Data'));

80-char wrap time. There might be more that I missed...

Also, lots of places to put @ingroup queue_example, and expand on docblock explanations.

m4olivei’s picture

Taking up the torch! First pass getting the module to enable, using FormStateInterface, correcting for current PSR-4 standards, using $this->t in the Form class, and getting rid of the theme function, since it was nothing but a simple wrapper for #theme => 'table' anyway. Still needs work, but good start for one night.

m4olivei’s picture

FileSize
12.29 KB

Forgot the interdiff.

m4olivei’s picture

Here's a new patch that finished bringing everything up to current standards and gets the example into a working state. Still more documentation needed.

m4olivei’s picture

I think it's also a good idea to work in an example like:

http://www.vdmi.nl/blog/creating-drupal-cron-queue-worker-drupal-8

Which shows how to create a queue worker plugin. Going to work on that and update the patch.

m4olivei’s picture

Scratch that, seems this is already done in cron_example module.

legovaer’s picture

Without looking into this issue, I started working on a queue_example module myself. At the point of creating a new issue for this module, I discovered this issue. However, I'm going to share my approach since it's a bit different (and IMHO simpler).

There is only a minor issue with my version: the testcases are failing. I've been looking into them several hours, but I can't get them to work. It would be nice of someone else can have a look.

legovaer’s picture

I've had a look at 2102703_11.patch and found a typo in /queue_example/queue_example.routing.yml:2:

path: 'examplse/queue_example' should be path: 'examples/queue_example'

m4olivei’s picture

Thanks, here is an updated patch correcting that typo.

I looked over your patch. It also demonstrates the primary functions of queue API. The one I have been working on is kinda nice to interact with all in one page, plus there is a handy run cron button so you don't have to leave the example to see the effect on the queues. I'm indifferent, either could work. My only concern with the patch in #14 is the use of ConfigFactoryInterface could be confusing. I know I was kinda thrown with that, b/c I know know nothing about it. Besides, I'm not sure how necessary it is in this example to keep what the processed items are around so that they can be shown. IMO it makes more sense to show the contents of the queue rather than what's been processed.

navneet0693’s picture

Status: Needs work » Needs review

The last submitted patch, 6: interdiff-do-not-test.diff, failed testing.

The last submitted patch, 9: 2102703_9.patch, failed testing.

The last submitted patch, 11: 2102703_11.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 16: 2102703_16.patch, failed testing.

The last submitted patch, 14: 2102703-14-different-implementation.patch, failed testing.

The last submitted patch, 16: 2102703_16.patch, failed testing.

navneet0693’s picture

Removing the errors thrown in #16 and adding menu entry.

Status: Needs review » Needs work

The last submitted patch, 24: port_queue_example-2102703-24.patch, failed testing.

The last submitted patch, 24: port_queue_example-2102703-24.patch, failed testing.

The last submitted patch, 24: port_queue_example-2102703-24.patch, failed testing.

navneet0693’s picture

The errors thrown on line by CI on line numbers 47, 48, 53, 54, 58 and 66 are not so on my local. However, after sending a day I found that somehow the "Run Cron manaully to expire claims" doesn't expires the queue and even doesn't updates the table, also there is no effect of "Claim the next item and delete it" once "Claim the next item from the queue" is clicked. I am still working to get a fix on this.

navneet0693’s picture

So, here's what I found something while I was hanging around IRC to find answer for why cron run doesn't releases the claimed Items : #2705809: Queue garbage collection is not correctly run on cron

navneet0693’s picture

Status: Needs review » Needs work

The last submitted patch, 30: port_queue_example-2102703-30.patch, failed testing.

The last submitted patch, 30: port_queue_example-2102703-30.patch, failed testing.

navneet0693’s picture

FileSize
54.04 KB

Tests are running successfully on my local, I will wait for someone to check it once on their systems.

navneet0693’s picture

Assigned: Unassigned » navneet0693
navneet0693’s picture

Status: Needs work » Needs review
Shreya Shetty’s picture

FileSize
31.58 KB

image
This patch works fine . I tested the queue module and ran the test at my instance resulting with 0 errors and passing all the tests cases . But its failing here . Now its upto co-maintainers to look into this issue.

Shreya Shetty’s picture

Status: Needs review » Reviewed & tested by the community
Torenware’s picture

Status: Reviewed & tested by the community » Needs review

@Shreya, let's make sure we're running on the test bot as well. Sometimes, you get something that runs on your local that does not run on the DrupalCI automatic test system. This is not so fun when it happens, but if it runs locally but not on the bot, we need to find what the problem is.

I'm going to look at navneet0693's patch and see if I can figure out why we are having trouble.

Torenware’s picture

+++ b/queue_example/src/Tests/QueueExampleTestCase.php
@@ -0,0 +1,80 @@
+class QueueExampleTestCase extends WebTestBase {
+  public static $modules = array('queue_example');
+

This one's a serious problem, since this test will NEVER run. Why? Because test classes must end with "Test". This class should be renamed to something like QueueExampleTest. I'll see if it passes if it really does run.

Since you folks are doing this during the New Orleans Sprint, you are now honorary New Orlean Sprinters!

Status: Needs review » Needs work

The last submitted patch, 39: port_queue_example-2102703-39.patch, failed testing.

navneet0693’s picture

Hi Torenware,

Thanks for guiding Shreya, and also to pointing out mistake. I am sure not sure, even after the test name doesn't ends with "Test", if somehow ran. The error thrown on previous and current patch are same. I was not able to figure it out, why it was going it like that. So, i left it for maintainer's review. :) Thanks for reviewing it.

Torenware’s picture

navneet0693, I"m not sure how it ran either; AFAIK the test discovery system insisted on tests being named that way. They also must have the @group annotation, which your test does have. That said: the test discovery code has changed quite a bit over the last couple of months, so perhaps the simpletest part of the test runner is more forgiving than the PHPUnit part.

navneet0693’s picture

Previous patched failed to apply, re-rolling.

navneet0693’s picture

Status: Needs review » Needs work

The last submitted patch, 44: port_queue_example-2102703-44.patch, failed testing.

navneet0693’s picture

Status: Needs review » Needs work

The last submitted patch, 46: port_queue_example-2102703-45.patch, failed testing.

Torenware’s picture

These are the kind of bugs I hate... I've installed your patch, and run run-tests.sh locally. And your test runs w/o error. So we have one of these weird and wonderful bugs that are specific to the test bot. Been there, done that, AND ALL I GOT WAS A LOUSY T-SHIRT (which is hopefully an expression in your version of English!). Really: just a T-shirt. I didn't even get my patch into core ;-)

The next step is to look at the output the bot creates, and see what makes sense. This might be a timing issue of some kind. In which case we can make the test wait a bit at the point we need it to. Worse case, we change the test to use a phpunit functional test, which uses a different way of exercising the form.

Torenware’s picture

A quick look at the bot logs indicate that the calls to ::drupalPostForm() from lines 47, 53 and 57 are failing with some consistency. This does not happen when run-tests.sh is run locally. I'm not sure why a simpletest form test would fail in this matter, but that's what the log indicates.

You have a couple choices as to how to proceed here.

1) You may want to try the #drupal-testing channel on IRC, to see if anyone has advice.

2) If it were me doing this, I'd try to do this as a PHPUnit functional test. This is very similar to the simpletest WebTestBase test you're doing now, but I think this newer format (which uses the Mink library) might be more robust, and less likely to do strange things when tested under the bot. I don't think that any examples have "landed" yet (there are a couple of good examples now in patches), so if you don't know how to do this, I can point you to where these are. In any case, one or the other patches will land soon, and an example will be easier to find when one of them does.

navneet0693’s picture

@Torenware Please help me out with those examples.

mradcliffe’s picture

  1. +++ b/queue_example/src/Forms/QueueExampleForm.php
    @@ -0,0 +1,380 @@
    +          t('Item ID'),
    +          t('Claimed/Expiration'),
    +          t('Created'),
    +          t('Content/Data'),
    

    Should use $this->t instead of t.

  2. +++ b/queue_example/src/Forms/QueueExampleForm.php
    @@ -0,0 +1,380 @@
    +    $result = $this->database->query('SELECT item_id, data, expire, created FROM queue WHERE name = :name ORDER BY item_id',
    

    Database table names should be enclosed with {} so that a Drupal installation using table prefixes work.

    This is why the error is happening in drupalci.

Torenware’s picture

@mradcliffe:

These certainly need doing; we'll see if it affects the bot issue....

OK, bad news and good news.

Bad news is that not only does this not fix the bot; it breaks running run-test.sh locally.
Good news is that it's a lot easier to debug these kinds of problems when running locally, and your changes make us die the same way we died on the bot.

Now we get to see what's up.

Torenware’s picture

Underlying cause is that somehow, the queue table is apparently *not* getting created:

GET request to: examples/queue_example
Ending URL: http://drupalvm.dev/examples/queue_example
The website encountered an unexpected error. Please try again later.

Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupal.test60996192queue' doesn't exist: SELECT item_id, data, expire, created FROM {queue} WHERE name = :name ORDER BY item_id; Array ( [:name] => queue_example_first_queue ) in Drupal\queue_example\Forms\QueueExampleForm->retrieveQueue() (line 212 of modules/examples/queue_example/src/Forms/QueueExampleForm.php).

I'm a little uncertain how WebTestBase goes about bootstrapping Drupal. My guess is that queue is not always there. Not sure why; my assumption was that WebTestBase did a full install to the needed profile level. Maybe not?

Mile23’s picture

Status: Needs work » Needs review
FileSize
19.91 KB
1.68 KB

Right, the table is not created before the queue has items in it. So let's check if there are items before we do the query. You know... USING THE API. :-)

Mile23’s picture

  1. +++ b/examples.module
    --- /dev/null
    +++ b/queue_example/queue_example.info.yml
    

    Needs to declare a dependency on examples module.

  2. +++ b/queue_example/src/Forms/QueueExampleForm.php
    @@ -207,11 +208,17 @@ class QueueExampleForm extends FormBase {
    +    if ($this->queueFactory->get($queue_name)->numberOfItems() >= 1) {
    

    We could also check if the queue item is an instance of the DatabaseQueue class.

  3. +++ b/queue_example/src/Forms/QueueExampleForm.php
    @@ -207,11 +208,17 @@ class QueueExampleForm extends FormBase {
    +      debug('made it.');
    

    Ewps. :-)

Torenware’s picture

We cover fot the case where the user is not using the standard queue implementation (we warn the form won't work right). Formatted to pass phpcs.

Torenware’s picture

OK, we're about done here. Peace now or forever hold your speak.

navneet0693’s picture

+++ b/queue_example/queue_example.info.yml
@@ -0,0 +1,8 @@
+

We just need to remove this extra white line, it doesn't looks while applying the patch, and we are good to go!

navneet0693’s picture

navneet0693’s picture

Assigned: navneet0693 » Unassigned

We just need RTBC now, I guess :P :)

  • Torenware committed e3f89fb on 8.x-1.x authored by navneet0693
    Issue #2102703 by navneet0693, tsphethean, m4olivei, Torenware, Mile23,...
Torenware’s picture

Status: Needs review » Fixed

Why go RBTC when you can go full commit? Thanks all (it's a long list) for getting this one in.

navneet0693’s picture

Awesome!!!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.