Problem/Motivation
Original parent issue's scope was too broad. Breaking the patch on the parent issue into smaller child issues.
@mmatsoo had found words 'not existing' and changed it to 'nonexistent'. After searching for more instances of 'not existing', I found use cases that are correct in context, such as 'Database connection failed for some other reason than the database not existing.', but found others that could be changed. I also found instances of 'non-existent'.
Remaining tasks
Decide to consistently use either 'nonexistent' or 'non-existent'.- Fix 'not existing' to decided case when it makes sense in context.
Proposed resolution
As per one of the Similar issue , we will change the less occurrence word to maintain the flow.
In core, the word “not existing” appears 17 times where 3 are grammatically ok so we need to change only 14. Whereas the word “non-existent” appears 111 times. Given that 'non-existent' already occurs more often than 'not existing', it makes sense for us to change to 'non-existent' in this patch.
Out of scope
Some instance like Database connection failed for some other reason than the database not existing.
can be ignored as they are grammatically correct.
Original snippet of code from parent issue's patch:
Comment | File | Size | Author |
---|---|---|---|
#43 | 3154909-43.patch | 12.61 KB | jungle |
#43 | interdiff-34-43.txt | 3.11 KB | jungle |
Comments
Comment #2
ju.vanderw CreditAttribution: ju.vanderw at Palantir.net commentedComment #3
ju.vanderw CreditAttribution: ju.vanderw at Palantir.net commentedComment #4
ju.vanderw CreditAttribution: ju.vanderw at Palantir.net commentedComment #5
rajandro CreditAttribution: rajandro as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedIn core, the word “not existing” appears 17 times where 3 are grammatically ok so we need to change only 14. Whereas the word “non-existent” appears 111 times, so I think as per https://www.drupal.org/project/drupal/issues/2898947#comment-13721819 we need to change “not existing” of 14 targeted word to “non-existent”.
Comment #6
rajandro CreditAttribution: rajandro as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #7
rajandro CreditAttribution: rajandro as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedPlease find the patch for the changes.
Thanks
Rajandro
Comment #8
rajandro CreditAttribution: rajandro as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #9
rajandro CreditAttribution: rajandro as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #10
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedDid a grep on the code base and could fine some more.
Replaced in modules/link/tests/src/Unit/Plugin/Validation/Constraint/LinkNotExistingInternalConstraintValidatorTest.php. Other places I felt that 'not existing' fits perfectly.
Comment #11
priyanka.sahni CreditAttribution: priyanka.sahni at Srijan | A Material+ Company for Drupal India Association commentedComment #12
priyanka.sahni CreditAttribution: priyanka.sahni at Srijan | A Material+ Company for Drupal India Association commentedVerified and tested by applying the patch #10.Patch was applied successfully.Did a search on the code base in some of path directory it still exists.
Before Patch -
After Patch -
Comment #13
priyanka.sahni CreditAttribution: priyanka.sahni at Srijan | A Material+ Company for Drupal India Association commentedComment #14
ju.vanderw CreditAttribution: ju.vanderw at Palantir.net commentedThis change is grammatically incorrect. It should be "non-existent field".
This sentence is not understandable and unclear. Let's take it out of this patch and file a follow-up issue to correct this comment.
Changing the status back to Needs Work.
Comment #15
sanjayk CreditAttribution: sanjayk as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #16
sanjayk CreditAttribution: sanjayk as a volunteer and at Srijan | A Material+ Company for Drupal India Association commented@ju.vanderw fixed first issue and will create a follow-up issue as you mentioned in comment.
Comment #17
ju.vanderw CreditAttribution: ju.vanderw at Palantir.net commentedThis should be "Tests groupby with a non-existent field on some bundle." The word "field" is duplicated.
Comment #18
adityasingh CreditAttribution: adityasingh as a volunteer and at Srijan | A Material+ Company for Drupal India Association commented@Hi ju.vanderw
Updated patch please review.
Comment #19
adityasingh CreditAttribution: adityasingh as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedPlease review patch #18.
Comment #20
sanjayk CreditAttribution: sanjayk as a volunteer and at Srijan | A Material+ Company for Drupal India Association commented@ju.vanderw #18patch looks good. I think all the issues fixed. It's for me +1 RTBC.
Comment #21
PapaGrandeThis should be "a non-existent".
Same here.
Comment #22
nitesh624Working on it will give status update in next 2 hours
Comment #23
nitesh624Updates :
Fixed the suggestion in #21
Comment #24
rajandro CreditAttribution: rajandro as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedHi @PapaGrande and @nitesh624,
Those two grammatical changes are already under progress with another ticket Fix grammar 'a' to 'an' when necessary. I think we can stick to the scope of the issue and the rest of the gramatical changes can be fixed under the parent ticket of the same.
Thanks !
Comment #25
nitesh624Sorry for the noise and thanks @rajandro for pointing this out. So patch #18 will fixes the scope of current issue. RTBC +1 for #18
Comment #26
tripurari CreditAttribution: tripurari commentedComment #27
tripurari CreditAttribution: tripurari commented@adityasingh,On Applying patch #18, getting error on local Drupal Instance 9.1,Needs Re-Roll Patch #18.
Comment #28
tripurari CreditAttribution: tripurari at Srijan | A Material+ Company for Drupal India Association commentedSorry for the Noise, please ignore #27.
@adityasingh, After applying Patch #18 on Drupal 9.1 local Instance ,changes looks ok but still found not existing texts in the core. Kindly check the code block for reference.
Thanks
Comment #29
rajandro CreditAttribution: rajandro as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedI think these three changes are out of scope.
This is a newly added comment and this needs to change.
Comment #30
adityasingh CreditAttribution: adityasingh as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedHi @tripurari thanks for review.
Hi @rajandro thanks for suggetion.
Updated patch as per suggested in #29. Please review.
Comment #31
mmatsoo CreditAttribution: mmatsoo at Chromatic commented@adityasingh I reviewed the patch in #30 and I'm uncertain of this part:
I believe the context of the test is to check for a non-existent view, using the view_name and the view_display_id. So it probably should read "Tests using view_name and view_display_id for a non-existent view."
Comment #32
jungleAfter reading the test, I would rewrite it to Tests non-existent view with view_name and view_display_id. instead of Tests using view_name and view_display_id for a non-existent view. suggested in #31
Comment #33
samiullah CreditAttribution: samiullah at Salsa Digital commentedI did the grep for non-existent before and after applying the patches.
Apart from few comments in tasks.php file rest of changes were made nicely
Patch applied successfully and changes look good
@jungle Waiting for automated test to Pass after that this can be moved to RTBC
Comment #34
jungle@samiullah, thanks for the review.
Addressing the above.
Comment #35
samiullah CreditAttribution: samiullah at Salsa Digital commented@jungle new patch does not produce any logs on running git grep "not existing"
This can be moved to rtbc if code review is also fine
Comment #36
jungleRe #35, the only code change is an assertion message, the rest are changes made to comments. Feel free to RTBC it, I am not eligible to RTBC myself.
Comment #37
samiullah CreditAttribution: samiullah at Salsa Digital commentedComment #39
jungleRandom fail
Comment #41
jungleRandom fail again.
Comment #42
catch'not existing' is correct here, 'non-existent' isn't.
It would have to be 'Database connection failed for some other reason than a non-existent database', or just leave it as is.
Comment #43
jungleThanks @catch!
Replaced with what you suggested "Database connection failed for some other reason than a non-existent database." in 3 occurrences/files.
Comment #44
jungleSetting back to RTBC. Apologize if this is counting as self-RTBC and set back to NR, please.
Comment #46
jungleRe-queued, random fail.
Comment #47
larowlanI think task is the right category here
Comment #49
catchCommitted b62e284 and pushed to 9.1.x. Thanks!