Problem/Motivation
Currently if you try to go to /batch?id=123 you get redirected to the homepage with a message stating "No active batch".
Proposed resolution
Since the batch doesn't exist Drupal should just emit a 404 instead.
Display an error message to the user.
Remaining tasks
Update the error message: see Comment #187.Update the "after" screenshot in the issue summary.- #193, code review and testing
User interface changes
Error message displayed, "No batch with ID 20 exists."
Before
There is a redirect to the home page:
After
There is a 404 response instead of a redirect, and the error message is updated.
API changes
None
Comment | File | Size | Author |
---|---|---|---|
#190 | 1986330-after-mr-13250.png | 147.93 KB | Bhanu951 |
#190 | 1986330-before-mr-13250.png | 250.07 KB | Bhanu951 |
#180 | 1986330-180.patch | 2.21 KB | ravi.shankar |
#178 | #1986330.png | 15.43 KB | DrDam |
#177 | 1986330-177.patch | 2.23 KB | immaculatexavier |
Issue fork drupal-1986330
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #1
skwashd CreditAttribution: skwashd commentedI tried this out locally and it completely changed the way I think about batch.module. Drupal 7.23(24) needs this!!!!!! The logic in this patch aligns with my world view.
Update: Grammar fix
Comment #2
alexandrezia CreditAttribution: alexandrezia commentedthanks for this patch,
worked for me, this was on my todo list for a long time!
Comment #3
caiovlp CreditAttribution: caiovlp commentedIt works perfectly and the code is beautifully written! I've been waiting for this since D3 ou D4... not sure, it's been a long time!
Comment #4
David_Rothstein CreditAttribution: David_Rothstein commentedThis looks like a good idea, but needs to go in Drupal 8 first.
Also, the code would need to exit or return something, since drupal_not_found() just prints HTML to the page but doesn't stop the rest of the code in the function from executing, at least I think...
Comment #5
kasperg CreditAttribution: kasperg commentedI'll take a look at this for D8.
Comment #6
kasperg CreditAttribution: kasperg commentedHere is a patch for D8.
A couple of considerations:
BatchController
callsbatch_get()
from form.inc via_batch_page()
. which is not immediately available in the test cases. I'm unsure about the right way to go about this.Comment #7
marcelodornelas CreditAttribution: marcelodornelas commentedHi, this is fixed for Drupal 8. Please review.
Comment #8
valthebaldI'm slightly in favor of patch from #7, since it doesn't change an error message. Also, I don't see a reason to use format_string() instead of t()
Comment #9
valthebald#7: Error message should be wrapped with t() to allow translatable messages
Comment #10
marcelodornelas CreditAttribution: marcelodornelas commentedIt's done. Wrapped the message with t().
Comment #11
marcelodornelas CreditAttribution: marcelodornelas commentedOk, version 3 attached. Removed the message and t() as they are not required.
Comment #12
marcelodornelas CreditAttribution: marcelodornelas commentedPlease see v3 attached. I removed the t() and message as it's not required.
Comment #13
marcelodornelas CreditAttribution: marcelodornelas commentedThis is fixed.
Comment #14
marcelodornelas CreditAttribution: marcelodornelas commentedThis is fixed.
Comment #15
kasperg CreditAttribution: kasperg commented#8: In regarding to changing the error message I think it makes a difference. Better exception messages makes for better debugging and with the current exception handling further up the tree the additional technical information is not exposed to the user.
I know the NotFoundHttpException is usually used without any message throughout Drupal but ViewPageController does this the same way as my suggested patch.
Anyways I'll set this to needs review again.
Comment #16
valthebaldIn the current 8.x, text exposed to exception constructor, is not used at all (this indeed may be opened as a separate issue). So, for now, patches from #14 and #6 have the same effect.
Future-proof, I would prefer to have original error text, wrapped in t(), and passed to NotFoundHttpException() constructor.
Comment #17
mgiffordComment #18
dawehnerWell therefore we would need to adapt our exception handling but I think we should really take that into account.
Comment #19
valthebaldThis issue needs issues summary update, and the fix itself looks like a good starter issue.
My suggestion is reroll of patch #6 + open followup issue for use of passed text in HttpNotFoundException
Comment #20
stefank CreditAttribution: stefank commentedOk, here is a reroll from #6.
Comment #21
valthebaldAdded #2372011: NotFoundHttpException handler should take into account it's parameters as a follow-up
Comment #22
alexpottlet's use the utility object here, ie. String::format()
Comment #23
tstoecklerIt would be awesome to use
String::format()
instead, but not setting to needs work for that.Comment #24
kasperg CreditAttribution: kasperg commentedHere is a update to #20 which uses
String::format()
.Comment #25
tstoecklerAwesome, thanks a lot!
It's just that there is an ongoing effort to replace the usage of deprecated functions so it's nice to not introduce new calls to them :-)
Comment #26
kasperg CreditAttribution: kasperg commentedNo worries. Happy to help.
Comment #27
alexpottIt'd be good to add a test for this functionality. Obviously we did not have one for the redirect as that would be failing.
Comment #28
subhojit777Comment #30
subhojit777I am writing tests for this. This is what I thought:
This is just an explanation about the test I am going to write. Please correct me if I am wrong. I am not much aware of simpletest or unit test :p :)
Thanks.
Comment #31
subhojit777Here is the patch with tests. Please review :) I hope the test is alright. Not removing the "Need tests" tag, as I am not sure the test case is right.
Comment #32
moymilo CreditAttribution: moymilo commentedI am going to review it and update the issue summary.
Comment #33
moymilo CreditAttribution: moymilo commentedComment #34
moymilo CreditAttribution: moymilo commentedComment #35
moymilo CreditAttribution: moymilo commentedTry it manually and read all the lines in the patch. Seems to work fine.
Comment #36
YesCT CreditAttribution: YesCT commentedwhy pick a random id?
Maybe we should pick one we know will not exist.
Also, I think we need a tests only patch here, to make sure it fails nicely with the testbot.
Comment #37
wheatpenny CreditAttribution: wheatpenny commentedSmall nitpick changes to documentation with new patch. These are shown in the interdiff
- "Id" to "ID"
- setUp should have {@inheritdoc} docblock
- empty line before the final } in the class: https://www.drupal.org/node/608152#indenting
- Summary needs to be one line: https://www.drupal.org/coding-standards/docs
Also adding a tests only patch to show that the patch works as intended.
Comment #38
dawehnerI would prefer something like 'Batch %id request, but not found.', there is no need to talk about 'Page controller' here.
Let's mark it to be a public function testBatchNotFound() already.
Comment #39
wheatpenny CreditAttribution: wheatpenny commented1. Removed assert message.
2. "Id" to "ID"
3. Added "public" to function.
Comment #40
YesCT CreditAttribution: YesCT commentedneeds work for #38 1.
and we should get an opinion on the random question from #36.
--
Just a note, about adding public visibility is from the standard: https://www.drupal.org/node/608152#visibility
And thanks for removing that assert message, it wasn't adding any information. the default assert message should be fine in this case.
Comment #42
subhojit777Comment #43
subhojit777Using an arbitrary batch id looks odd. It would be good if we are able to get last batch id, and then increment it by any number (say 1), and use that batch id for testing.
But in any case we are getting page not found, even with batch ids that have already been used.
Suggestion:
Start a batch process within test. Use db_next_id() to retreive the next batch id, and then use it for "batchnotfound" test.
Comment #44
subhojit777I have tried this code for starting a batch process and doing db_next_id() to get next batch id.
But I am getting this error message.
This may be because there is an ongoing batch process which has not finished. That is why I have used
sleep(120)
, but I am still getting that error.Any idea regarding this? Or is it the right way for starting batch process inside tests.
Invoking test scripts for #43
Comment #45
subhojit777We can create a dummy user and delete it. The cancellation process of user will be a batch process. This way we can replicate batch process inside test. Writing a patch for this.
Comment #46
subhojit777A test account is created and deleted. The deletion process will be a batch process. We then use db_next_id() to retrieve a non-existent batch id, and then use it for our test case. I hope this logic is enough for the test case.
Comment #48
subhojit777I do not understand.. The same patch is passing the test in localhost.
Comment #49
subhojit777Uploading patch with empty
$edit
in test. This patch passed clean in my localhost. I hope it passes here too...Comment #50
subhojit777Patch without fix. This patch should fail testing.
Comment #52
subhojit777Desired result obtained. Moving this to "Needs review".
Comment #53
subhojit777Comment #54
AkshayKalose CreditAttribution: AkshayKalose commented#49 gives 404 error as desired on visiting /batch?id=123 However, message 'Batch 123 requested, but not found.' does not show up.
Comment #55
subhojit777Comment #56
subhojit777Comment #57
subhojit777It is only an exception message.
The 404 page is rendered from this code in
core/modules/system/src/Controller/Http4xxController.php
I guess this is what happening, though I would strongly ask some Drupal 8 expert guy would review this.
Also refer Symfony docs: http://api.symfony.com/2.1/Symfony/Component/HttpKernel/Exception/NotFoundHttpException.html
Comment #58
subhojit777*bump* anybody wants to do review?
Comment #59
valthebald@subhojit777: can you please add Beta changes evaluation section?
Comment #60
DevElCuy CreditAttribution: DevElCuy commentedI looked at the test-only patch at #50, read all the lines, the test seems
relevant to the test. Only thing to fix is the naming conventing for the class property.
https://www.drupal.org/node/608152#naming
"Methods and class properties should use lowerCamel naming"
The testbot is raising only on failure, which is good, since it demonstrates the bug exists.
Next, actually right now, I'm going to read whole patch and provide feedback.
Comment #61
DevElCuy CreditAttribution: DevElCuy commentedComment #62
DevElCuy CreditAttribution: DevElCuy commentedComment #63
DevElCuy CreditAttribution: DevElCuy commentedAfter reviewing the whole patch at #49 and read the comment at #57, it makes sense to use default Drupal behavior than throwing a custom 404, which is not the case.
Comment #64
DevElCuy CreditAttribution: DevElCuy commentedComment #65
subhojit777Comment #66
subhojit777I will add the beta changes now.
Comment #67
subhojit777I think develCuy has already added the beta evaluation changes. Thanks!
Comment #70
subhojit777Comment #71
vitorsdcs CreditAttribution: vitorsdcs commentedAs develCuy has already said at #63, we should avoid using a custom 404 message in /core/includes/batch.inc. Instead, there is a function called on404() in /core/modules/system/src/Controller/Http4xxController.php which could be used to display a default 404 message. The problem is that this function is in a module, and as we need to use it inside batch.inc, it should be part of core. Bottom line is, we must solve this architectural problem and then use on404() for the 404 message.
Comment #72
YesCT CreditAttribution: YesCT commentedat first I thought maybe do something like
but then I searched for on404
so it seems this is not directly called anywhere in core.
Maybe that is because it is triggered by an event.
I'm not sure what to do here. Maybe we want to trigger an event?
Comment #73
DevElCuy CreditAttribution: DevElCuy commented@Crell managed to review why in world core/modules/system/src/Controller/Http4xxController.php exists, and explained me that is being used somewhere else and we don't need to care about it, so let's forget about on404() since it has nothing to do here!
Comment #74
DevElCuy CreditAttribution: DevElCuy commentedFollowing #73, the patch at #66 is good, so changed my mind about #63, since the custom message added by the patch makes A LOT of sense for the particular context of this issue.
Comment #76
webchickWell this certainly makes a great deal more sense. Looks like Alex's feedback has been addressed.
Committed and pushed to 8.0.x. Thanks!
Comment #78
xjm@webchick reverted this because the added test had a high random fail rate:
I didn't determine what was causing the random failure, but this looks really suspect:
Comment #79
xjmAlso (unrelated), it would be better to not couple this test to Views. We should look in existing batch tests to see how test batches have been implemented previously.
Comment #80
xjmFor whoever works on this patch, you can run the test repeatedly locally to check whether it's failing randomly:
And to debug the result that failed:
Comment #81
richardcanoe CreditAttribution: richardcanoe commentedThanks @xjm for the pointer on how to run the tests. I found the bug and fixed it so the BatchNotFoundTest runs correctly.
With regard to #79 I have left the reference to views in since this is a 'real world' test using people admin pages which require views. In D7 there was a batch_test module which created test based scenarios and batches, only one of which, testPercentages, is recreated in D8. If these tests are all to be recreated for D8 then it is perhaps outside the scope of this issue.
Comment #82
YesCT CreditAttribution: YesCT commentednot novice
Comment #83
YesCT CreditAttribution: YesCT commentedthank you.
I wanted to see what the fix was, so I made an interdiff.
For instructions on creating an interdiff, see https://drupal.org/documentation/git/interdiff
Interdiffs are really helpful to do. it makes it much more likely to get reviews and feedback.
New tests we add are to be done better than we have in the past.
I agree, there should be a way to test this that does not involve views. need work for that.
Comment #84
subhojit777Lets see what I can do.
Comment #85
subhojit777Just curious - Why can't we use views for the tests.
I just saw there is are tests called
ProcessingTest.php
,PageTest.php
which involvesbatch_test
module. I was thinking of using the same logic, but it *also* enables a module.Comment #86
subhojit777I have re-wrote the test using
batch_test
module .Comment #94
quietone CreditAttribution: quietone as a volunteer commentedThe latest patch addresses all the issues raised. It needs a reroll and it is a suitable novice task.
Comment #95
shashikant_chauhan CreditAttribution: shashikant_chauhan as a volunteer and at Iksula commentedAdding patch.
Comment #96
shashikant_chauhan CreditAttribution: shashikant_chauhan as a volunteer and at Iksula commentedComment #98
shashikant_chauhan CreditAttribution: shashikant_chauhan as a volunteer and at Iksula commentedComment #100
quietone CreditAttribution: quietone as a volunteer commented@shashikant_chauhan, thanks for the reroll! Please add an interdiff, it makes it a lot easier to review.
I tracked down the String error. According to #2476075: Remove usage of \Drupal\Component\Utility\String::format() String::format has been removed and replaced with \Drupal\Component\Utility\SafeMarkup::format(). And string is a reserved word in php7 #2454447: Split Utility\String class to support PHP 7 (String is a reserved word).
Comment #101
nikunjkotechaComment #102
nikunjkotechaUpdated patches to use SafeMarkup instead of String.
Comment #103
nikunjkotechaCreated separate issue for D7 backport - https://www.drupal.org/node/2826866#comment-11776511
Comment #104
quietone CreditAttribution: quietone as a volunteer commentedScreen shot of http://d8.local/batch?id=4
Screen shot of http://d8.local/xyzzy
Does it matter about the different theme for the batch URL and the non batch URL?
Comment #105
valthebaldBumping version
Comment #106
mangy.fox CreditAttribution: mangy.fox at Investis Digital commentedTested on 8.3.x using Simplytest.me, patch still working.
Comment #107
mangy.fox CreditAttribution: mangy.fox at Investis Digital commentedAlso re-ran tests against 8.3.x.
Comment #108
alexpottWe need a change record for this change. Also we need to consider if this will break any expectations - I don't think so.
Comment #109
mangy.fox CreditAttribution: mangy.fox at Investis Digital commentedAdded draft change record - https://www.drupal.org/node/2842748. First time I've done one, so hope it's ok.
Comment #110
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedWe can give a better error message:
Comment #111
cebasqueira CreditAttribution: cebasqueira at CI&T commentedComment #113
cebasqueira CreditAttribution: cebasqueira at CI&T commentedComment #114
dawehnerThat test seems to be in the wrong location. The tests there should extend
BrowserTestBase
and notWebTestBase
Comment #118
aron.beal CreditAttribution: aron.beal as a volunteer commentedRerolled test into BrowserTestBase, passes.
Comment #119
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedI am not why this one is marked as Patch to be Ported @aron.beal, I am changing it to needs Needs Review for the tests to run.
Comment #121
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedNow we know why the first patch patch failed and second passed as first is test only! This should be clear to go :) marking it as RTBC
Comment #122
aron.beal CreditAttribution: aron.beal as a volunteer commentedHi, yes. Participating in a sprint yesterday, still a little unclear as to all the protocols for getting patches committed, but I was told by someone there that uploading tests only as a separate patch gave thread reviewers a clearer picture that the patch fixed the issue the test was testing. @navneet0693, in a similar vein, I thought because I was making changes that it should have been marked as a separate patch. I'll leave it as 'needs review' next time.
Comment #123
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedhey @aron.beal thanks for efforts and patch :)
Comment #124
xjmWe should not use SafeMarkup in exception messages. Actually we should not use it at all, because it is deprecated. But either way, see here for the correct usage: https://www.drupal.org/node/608166
Thanks!
Comment #125
nikunjkotechaUpdated patch to use sprintf and removed unwanted includes.
Comment #126
nikunjkotechare-adding the same to add test, not sure if a potential bug but I did preview before saving and tests were not added.
Edit: Test not added again, not sure why.
Comment #127
nikunjkotechaComment #128
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedChanging to "Needs Review" for the given tests to run.
Comment #129
nikunjkotechaThanks @Navneet.
Comment #130
quietone CreditAttribution: quietone as a volunteer commentedAdded the error message to the User interface changes section in the IS. Changes have been made for all the suggestions so nearly there.
Just two questions and an action for the change record.
For such a simple message why not do this?
throw new NotFoundHttpException(sprintf('Trying to execute an invalid batch ID : %s.', $request_id))
dawehner stated in #114 that the test was in the wrong location. Is this the correct location?
The change record needs to be updated to use the latest version of the error message.
Note that the patch in #125 is identical to the one in #126.
Comment #131
shashikant_chauhan CreditAttribution: shashikant_chauhan as a volunteer and at Iksula commentedCleaning the tags. We are working on this issue.
Comment #132
shashikant_chauhan CreditAttribution: shashikant_chauhan as a volunteer and at Iksula commentedAdded the change #1 suggested by quietone.
Test location looks correct to me.
Comment #133
quietone CreditAttribution: quietone as a volunteer commentedAfter installing the patch and navigating to /batch?id=123 I get "The requested page could not be found." instead of the new error message.
Comment #134
shashikant_chauhan CreditAttribution: shashikant_chauhan as a volunteer and at Iksula commentedI tried but could not find anything why drupal is showing its default 404 message instead of new message passed to it.
@quietone, Any idea how can I fix this issue.
Comment #135
nikunjkotecha@quietone
I suggest we add a Drupal message if we really want to display the error message.
Checked the source, message is not used anywhere in NotFoundHttpException class, it could be used if the exception was really caught and $ex->message was read but here we simply display the 404 page.
Comment #140
vacho CreditAttribution: vacho at Skilld commentedComment #141
vacho CreditAttribution: vacho at Skilld commentedPatch rerolled. However when I review, I noticed that this problem is already solved (please view interdiff) in code. So this patch is only the test for the feature.
Comment #142
vacho CreditAttribution: vacho at Skilld commentedSorry. comment #141 has wrong patch.
Patch rerolled. However when I review, I noticed that this problem is already solved (please view interdiff) in code. So this patch is only the test for the feature.
Comment #143
mangy.fox CreditAttribution: mangy.fox at Investis Digital commentedI have re-added the Drupal message, as the exception message was not being used.
Also updated the test to meet current standards.
Comment #144
selva.swamy@gmail.com CreditAttribution: selva.swamy@gmail.com as a volunteer and commentedWould this patch also handle the scenario where user hits /batch?id=123&op=start. This happens when users try to navigate to a batch page by clicking the back button of the browser.
Comment #145
John Cook CreditAttribution: John Cook at Creode commentedThank you everyone for working on this.
@kswamy, this patch does work with
/batch?id=123&op=start
. The 'op' parameter is not used when finding the batch in Drupal, only the 'id'. This means that setting or changing the value of 'op' does not affect the result.Unfortunately, this still needs work. The error message displayed on the site is 'Trying to execute an invalid batch ID : {ID}.' but the change record states that the message will be 'Batch {ID} requested, but not found.' ([#2842748])
The message from the change record is better as it moves the emphasis on to Drupal rather than the user. ('Invalid' means the user did something wrong, but 'not found' means Drupal did something wrong.)
Even so, that message is doesn't flow well. Maybe something like 'The requested batch ({ID}) could not be found.' would be better?
This would need to be changed:
All these can be novice tasks.
Otherwise this look ready to go.
Comment #146
smagdits CreditAttribution: smagdits at Mindgrub Technologies commentedI've updated the messages in the previous patch #143 as I agree that the suggested message sounds more natural.
Comment #147
John Cook CreditAttribution: John Cook at Creode commentedHi @smagdits.
Thank you having a go at this.
It looks like your patch has some extra code that wasn't in the previous one which is why the patch is failing. Maybe you were working on another issue?
Thanks for updating the issue summary. Now we all know what we are expecting the final patch to do.
Comment #148
John Cook CreditAttribution: John Cook at Creode commentedComment #149
smagdits CreditAttribution: smagdits at Mindgrub Technologies commentedMy apologies, looks like you're right about the extra code being from a different issue. I'll be sure to check my patches more thoroughly in the future, thank you. I've remade the patch and tested applying it.
Comment #150
John Cook CreditAttribution: John Cook at Creode commentedThanks @smagdits.
There's still a few more things to do.
The message by testing is "The requested batch {ID} could not be found." but the issue summary states that it should be "The requested batch ({ID}) could not be found." - the brackets are missing.
There's a coding standards error. "Short array syntax must be used to define arrays" - this should be
public static $modules = ['batch_test'];
Back to needs work for these points.
Comment #151
andriansyah CreditAttribution: andriansyah as a volunteer commentedHi @john-cook,
updated with the point no 1 and 2.
Comment #152
andriansyah CreditAttribution: andriansyah as a volunteer commentedComment #153
John Cook CreditAttribution: John Cook at Creode commentedThank you @andriansyah for working on this.
Points 1 and 2 from #150 have been addressed.
I think the code is done now.
The only thing left is to alter the change record so that it states what is shown.
I've added the "documentation" tag and set back to "Needs work" because of this.
Comment #154
andriansyah CreditAttribution: andriansyah as a volunteer and at Alterra commentedThe documentation for the change record has been updated using 'The requested batch ({ID}) could not be found.'
Comment #155
John Cook CreditAttribution: John Cook at Creode commentedAll the points from #150 have now been done.
Marking as RTBC.
Comment #156
quietone CreditAttribution: quietone as a volunteer commentedWanted to see what this looked like and since there are no screen shots I added them to the IS.
Two questions 1) should the page be themed, I found it a surprise to get a mostly white screen. And 2) Should the batch id be surrounded by parenthesis or single quotes? I only know that in error message single quotes are used for the variable and I was wondering if it is the same in UI text. I looked at Interface text but didn't see anything. Could have missed it.
Comment #159
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedI have re-rolled patch #151 on Drupal 8.9.x-dev
Comment #160
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedComment #162
AkashKumar07 CreditAttribution: AkashKumar07 at OpenSense Labs commentedAdd default theme in BatchNotFoundTest.php class
/**
* {@inheritdoc}
*/
protected $defaultTheme = 'stark';
Hope this will help you.
Comment #163
rajeevkRe-rolling patch for 8.9.x-dev
Comment #164
quietone CreditAttribution: quietone as a volunteer commentedThis should be {@inheritdoc}
Comment #165
Suresh Prabhu Parkala CreditAttribution: Suresh Prabhu Parkala at Specbee commentedChanged to {@inheritdoc}. Here is the patch.
Comment #166
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedHere I have made changes as suggested in comment #164.
Comment #167
quietone CreditAttribution: quietone as a volunteer commentedThanks for the patch.
Still one more question.
The error message is slightly different than the exception message. Should they be the same? What is 'correct'?
Since this is making a change visible to the user, lets get a usability review and perhaps that will answer this question too.
Comment #172
larowlanComment #174
immaculatexavier CreditAttribution: immaculatexavier as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedRerolled #166 patch against 9.5.x
Attached patch and reroll diff
Comment #175
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedRemoved needs reroll tag, and fixed Drupal CS issue of patch 174.
Comment #176
immaculatexavier CreditAttribution: immaculatexavier as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedRerolled patch against #174
Comment #177
immaculatexavier CreditAttribution: immaculatexavier as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedRerolled patch against #176 . Fixed custom commands
Comment #178
DrDam CreditAttribution: DrDam commentedOk against 9.4.x and 9.5.x
Comment #179
alexpottThe patch is still not passing the code checks.
Comment #180
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedAddressed Drupal CS issue of patch #177.
Comment #182
bebalachandra CreditAttribution: bebalachandra as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedTested patch #180 on drupal 10.1.
patch working fine
Comment #183
bebalachandra CreditAttribution: bebalachandra as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #184
larowlanAdding issue credits for those who changed the shape/course of the patch and/or made issue summary updates
Comment #185
larowlanPutting back to needs review because @quietone asked for a usability review in #167
I've pinged the UX team, I think this is ready to go code wise so would be good to get a +1 from the UX team on the wording
Comment #186
larowlanSlack thread where I pinged the UX team https://drupal.slack.com/archives/C1AFW2ZPD/p1668983795995769
Comment #187
benjifisherUsability review
We discussed this issue at #3322499: Drupal Usability Meeting 2022-11-25. That issue has a link to a recording of the meeting.
For the record, the attendees at the usability meeting were @AaronMcHale, @benjifisher, @ravi.shankar, @rkoller, and @simohell.
We considered whether 404 is the correct response code and agreed that it is. See RFC 7231: it does not matter, as we initially thought, whether the batch ID is in the path or the query string.
If we can figure out some common scenarios when this comes up, as in Comment #144, then it would be nice to give a more helpful message. That will require some discussion, so it should get its own issue. We do not want to hold up this one.
The only change we would like to see for this issue is to replace the error message. Instead of "The requested batch (20) could not be found", either of these is slightly shorter and more consistent with other Drupal error messages:
I am setting the status to NW for that change, and to update the screenshots.
I am also removing the "Beta phase evaluation" section from the issue summary and updating the remaining tasks.
Comment #190
Bhanu951 CreditAttribution: Bhanu951 as a volunteer commentedComment #191
Bhanu951 CreditAttribution: Bhanu951 as a volunteer commentedComment #192
Bhanu951 CreditAttribution: Bhanu951 as a volunteer commentedComment #193
benjifisher@Bhanu951:
Thanks for implementing the changes that the usability team recommended (Comment #187).
I am curious about the "after" screenshot: is the text "Error message" normally visible in the messages area, or did you use browser tools to un-hide it?
I think the usability concerns have been addressed. This issue now needs code review and testing.
I am updating the issue summary:
Comment #194
Bhanu951 CreditAttribution: Bhanu951 as a volunteer commented@benjifisher :
Thanks for the review.
I haven't used any browser tools. It is being displayed as shown in the image.
Comment #195
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.
Tested on Drupal 10.1.x with a standard install
Confirmed going to /batch?123 I get the "No active batch" error
Applying the fix
Going to /batch?123 I get a 404 page with "No batch with ID 123 exists."
Reviewing the change record and the change being addressed is clearly states.
Looks good
Comment #196
quietone CreditAttribution: quietone at PreviousNext commentedI agree with #193 that this needs a code review.
Comment #197
Akhil Yadav CreditAttribution: Akhil Yadav commentedadded patch against #180 in 10.1 version
Comment #198
BramDriesenHiding patch #197 as it's incomplete and omitting important changes.
Adding to the list #3339883: Employees of Dotsquares are posting mass re-roll patches which are invalid and/or incomplete
Comment #199
quietone CreditAttribution: quietone at PreviousNext commented@Akhil Yadav, It is expected that contributors read the issue before making comments etc. The MR is against 10.1.x. I am removing credit.