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.
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 ;)
Comment | File | Size | Author |
---|---|---|---|
#59 | interdiff-58.txt | 288 bytes | navneet0693 |
#59 | 2102703-58.patch | 21.21 KB | navneet0693 |
| |||
#56 | interdiff-56.txt | 9.78 KB | Torenware |
#56 | 2102703-56.patch | 21.21 KB | Torenware |
| |||
#54 | interdiff_46.txt | 1.68 KB | Mile23 |
Comments
Comment #1
tsphethean CreditAttribution: tsphethean commentedComment #2
tsphethean CreditAttribution: tsphethean commentedDirect port from D7 version, with simpletests passing locally
Comment #3
sdelbosc CreditAttribution: sdelbosc commentedCode looks good to me. Only few remarks:
Comment #4
tsphethean CreditAttribution: tsphethean commentedThanks for the review.
Good spot - thanks. Fixed
It was, because I implemented FormInterface instead of extending FormBase - now correctly extending FormBase and removed validateForm()
Good spot - thanks. Fixed.
Revised patch attached.
Comment #5
Mile23Woops, wrong cut and paste. :-) This is probably in the D7 version, too, I bet.
80 char wrap, please.
Comment #6
tsphethean CreditAttribution: tsphethean commented@Mile23 thanks for the review. I've fixed the two comments in the new patch.
Comment #7
Mile236: 2102703-6-port_queue_examples_d8.patch queued for re-testing.
Comment #8
Mile23Thanks, tsphethean.
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.
Comment #9
m4oliveiTaking 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.
Comment #10
m4oliveiForgot the interdiff.
Comment #11
m4oliveiHere's a new patch that finished bringing everything up to current standards and gets the example into a working state. Still more documentation needed.
Comment #12
m4oliveiI 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.
Comment #13
m4oliveiScratch that, seems this is already done in cron_example module.
Comment #14
legovaerWithout 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.
Comment #15
legovaerI'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 bepath: 'examples/queue_example'
Comment #16
m4oliveiThanks, 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.
Comment #17
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedComment #24
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedRemoving the errors thrown in #16 and adding menu entry.
Comment #28
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedThe 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.
Comment #29
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedSo, 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
Comment #30
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedComment #33
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedTests are running successfully on my local, I will wait for someone to check it once on their systems.
Comment #34
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedComment #35
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedComment #36
Shreya Shetty CreditAttribution: Shreya Shetty commentedThis 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.
Comment #37
Shreya Shetty CreditAttribution: Shreya Shetty commentedComment #38
Torenware CreditAttribution: Torenware as a volunteer commented@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.
Comment #39
Torenware CreditAttribution: Torenware as a volunteer commentedThis 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!
Comment #41
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedHi 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.
Comment #42
Torenware CreditAttribution: Torenware as a volunteer commentednavneet0693, 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.
Comment #43
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedPrevious patched failed to apply, re-rolling.
Comment #44
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedComment #46
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedComment #48
Torenware CreditAttribution: Torenware as a volunteer commentedThese 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.
Comment #49
Torenware CreditAttribution: Torenware as a volunteer commentedA 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.
Comment #50
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commented@Torenware Please help me out with those examples.
Comment #51
mradcliffeShould use
$this->t
instead oft
.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.
Comment #52
Torenware CreditAttribution: Torenware as a volunteer commented@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.
Comment #53
Torenware CreditAttribution: Torenware as a volunteer commentedUnderlying cause is that somehow, the queue table is apparently *not* getting created:
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?
Comment #54
Mile23Right, 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. :-)
Comment #55
Mile23Needs to declare a dependency on examples module.
We could also check if the queue item is an instance of the DatabaseQueue class.
Ewps. :-)
Comment #56
Torenware CreditAttribution: Torenware as a volunteer commentedWe 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.
Comment #57
Torenware CreditAttribution: Torenware as a volunteer commentedOK, we're about done here. Peace now or forever hold your speak.
Comment #58
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedWe just need to remove this extra white line, it doesn't looks while applying the patch, and we are good to go!
Comment #59
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedComment #60
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedWe just need RTBC now, I guess :P :)
Comment #62
Torenware CreditAttribution: Torenware as a volunteer commentedWhy go RBTC when you can go full commit? Thanks all (it's a long list) for getting this one in.
Comment #63
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedAwesome!!!