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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ju.vanderw created an issue. See original summary.

ju.vanderw’s picture

Issue summary: View changes
ju.vanderw’s picture

Assigned: ju.vanderw » Unassigned
Issue summary: View changes
FileSize
221.26 KB
ju.vanderw’s picture

Title: Change words 'not Existing' to 'nonexistent' » Change words 'not existing' to 'nonexistent'
rajandro’s picture

Assigned: Unassigned » rajandro

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, 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”.

rajandro’s picture

Title: Change words 'not existing' to 'nonexistent' » Change words 'not existing' to 'non-existent'
Issue summary: View changes
Related issues: +#2898947: Change "writeable" to "writable" in documentation
rajandro’s picture

Please find the patch for the changes.

Thanks
Rajandro

rajandro’s picture

Issue summary: View changes
rajandro’s picture

Assigned: rajandro » Unassigned
Status: Needs work » Needs review
mohrerao’s picture

Did a grep on the code base and could fine some more.

 grep -ri "not existing" *
lib/Drupal/Core/Database/Driver/sqlite/Install/Tasks.php:        // not existing.
lib/Drupal/Core/Database/Driver/mysql/Install/Tasks.php:        // not existing.
lib/Drupal/Core/Database/Driver/pgsql/Install/Tasks.php:        // not existing.
modules/link/tests/src/Unit/Plugin/Validation/Constraint/LinkNotExistingInternalConstraintValidatorTest.php:    // Not existing routed URL.
tests/Drupal/KernelTests/Core/Entity/EntityFieldTest.php:    $this->assertFalse(isset($entity->nameInvalid), new FormattableMarkup('%entity_type: Not existing field is not set.', ['%entity_type' => $entity_type]));

Replaced in modules/link/tests/src/Unit/Plugin/Validation/Constraint/LinkNotExistingInternalConstraintValidatorTest.php. Other places I felt that 'not existing' fits perfectly.

priyanka.sahni’s picture

Assigned: Unassigned » priyanka.sahni
priyanka.sahni’s picture

FileSize
436.72 KB
1.1 MB

Verified 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 -
Before Patch

After Patch -
After Patch

priyanka.sahni’s picture

Assigned: priyanka.sahni » Unassigned
ju.vanderw’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/views/tests/src/Kernel/QueryGroupByTest.php
    @@ -294,7 +294,7 @@ public function testGroupByFieldWithCardinality() {
    +   * Tests groupby with a field non-existent on some bundle.
    

    This change is grammatically incorrect. It should be "non-existent field".

  2. +++ b/core/modules/views/tests/src/Unit/Controller/ViewAjaxControllerTest.php
    @@ -126,7 +126,7 @@ public function testMissingViewName() {
    +   * Tests with view_name and view_display_id but non-existent view.
    

    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.

sanjayk’s picture

Assigned: Unassigned » sanjayk
sanjayk’s picture

@ju.vanderw fixed first issue and will create a follow-up issue as you mentioned in comment.

ju.vanderw’s picture

+++ b/core/modules/views/tests/src/Kernel/QueryGroupByTest.php
@@ -294,7 +294,7 @@ public function testGroupByFieldWithCardinality() {
+   * Tests groupby with a field non-existent field on some bundle.

This should be "Tests groupby with a non-existent field on some bundle." The word "field" is duplicated.

adityasingh’s picture

@Hi ju.vanderw
Updated patch please review.

adityasingh’s picture

Status: Needs work » Needs review

Please review patch #18.

sanjayk’s picture

Assigned: sanjayk » Unassigned

@ju.vanderw #18patch looks good. I think all the issues fixed. It's for me +1 RTBC.

PapaGrande’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/views/tests/src/Unit/ViewsDataTest.php
    @@ -480,7 +480,7 @@ public function testCacheCallsWithWarmCacheAndDifferentTable() {
    +   * Tests the cache calls for an non-existent table:
    

    This should be "a non-existent".

  2. +++ b/core/modules/views/tests/src/Unit/ViewsDataTest.php
    @@ -518,7 +518,7 @@ public function testCacheCallsWithWarmCacheAndInvalidTable() {
    +   * Tests the cache calls for an non-existent table:
    

    Same here.

nitesh624’s picture

Assigned: Unassigned » nitesh624

Working on it will give status update in next 2 hours

nitesh624’s picture

Assigned: nitesh624 » Unassigned
Status: Needs work » Needs review
FileSize
8.18 KB
608 bytes

Updates :
Fixed the suggestion in #21

rajandro’s picture

Hi @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 !

nitesh624’s picture

Sorry for the noise and thanks @rajandro for pointing this out. So patch #18 will fixes the scope of current issue. RTBC +1 for #18

tripurari’s picture

Assigned: Unassigned » tripurari
tripurari’s picture

Assigned: tripurari » Unassigned
FileSize
1.43 MB

@adityasingh,On Applying patch #18, getting error on local Drupal Instance 9.1,Needs Re-Roll Patch #18.

tripurari’s picture

Sorry 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

core/lib/Drupal/Core/Database/Driver/mysql/Install/Tasks.php:
  161:         // not existing.

core/lib/Drupal/Core/Database/Driver/pgsql/Install/Tasks.php:
  111:         // not existing.

core/lib/Drupal/Core/Database/Driver/sqlite/Install/Tasks.php:
  107:         // not existing.

core/tests/Drupal/KernelTests/Core/Entity/EntityFieldTest.php:
  209:     $this->assertFalse(isset($entity->nameInvalid), new FormattableMarkup('%entity_type: Not existing field is not set.', ['%entity_type' => $entity_type]));
rajandro’s picture

core/lib/Drupal/Core/Database/Driver/mysql/Install/Tasks.php:
  161:         // not existing.

core/lib/Drupal/Core/Database/Driver/pgsql/Install/Tasks.php:
  111:         // not existing.

core/lib/Drupal/Core/Database/Driver/sqlite/Install/Tasks.php:
  107:         // not existing.

I think these three changes are out of scope.

core/tests/Drupal/KernelTests/Core/Entity/EntityFieldTest.php:
  209:     $this->assertFalse(isset($entity->nameInvalid), new FormattableMarkup('%entity_type: Not existing field is not set.', ['%entity_type' => $entity_type]));

This is a newly added comment and this needs to change.

adityasingh’s picture

Hi @tripurari thanks for review.
Hi @rajandro thanks for suggetion.
Updated patch as per suggested in #29. Please review.

mmatsoo’s picture

@adityasingh I reviewed the patch in #30 and I'm uncertain of this part:

-   * Tests with view_name and view_display_id but not existing view.
+   * Tests with view_name and view_display_id but non-existent view.

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."

jungle’s picture

  public function testMissingView() {
    $request = new Request();
    $request->request->set('view_name', 'test_view');
    $request->request->set('view_display_id', 'page_1');

    $this->viewStorage->expects($this->once())
      ->method('load')
      ->with('test_view')
      ->will($this->returnValue(FALSE));

    $this->expectException(NotFoundHttpException::class);
    $this->viewAjaxController->ajaxView($request);
  }
+++ b/core/modules/views/tests/src/Unit/Controller/ViewAjaxControllerTest.php
@@ -126,7 +126,7 @@ public function testMissingViewName() {
-   * Tests with view_name and view_display_id but not existing view.
+   * Tests with view_name and view_display_id but non-existent view.

+++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDefinitionUpdateTest.php
@@ -376,7 +376,7 @@ public function testBundleFieldCreateUpdateDeleteWithoutData() {
-   * This tests deletion when there are existing entities, but not existing data

After 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

samiullah’s picture

I 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

jungle’s picture

@samiullah, thanks for the review.

$ git grep "not existing"
core/lib/Drupal/Core/Database/Driver/mysql/Install/Tasks.php:        // not existing.
core/lib/Drupal/Core/Database/Driver/pgsql/Install/Tasks.php:        // not existing.
core/lib/Drupal/Core/Database/Driver/sqlite/Install/Tasks.php:        // not existing.

Addressing the above.

samiullah’s picture

@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

jungle’s picture

+++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityFieldTest.php
@@ -206,7 +206,7 @@ protected function doTestReadWrite($entity_type) {
-    $this->assertFalse(isset($entity->nameInvalid), new FormattableMarkup('%entity_type: Not existing field is not set.', ['%entity_type' => $entity_type]));
+    $this->assertFalse(isset($entity->nameInvalid), new FormattableMarkup('%entity_type: Non-existent field is not set.', ['%entity_type' => $entity_type]));

Re #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.

samiullah’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 34: 3154909-34.patch, failed testing. View results

jungle’s picture

Status: Needs work » Reviewed & tested by the community

Random fail

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 34: 3154909-34.patch, failed testing. View results

jungle’s picture

Status: Needs work » Reviewed & tested by the community

Random fail again.

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Install/Tasks.php
@@ -108,7 +108,7 @@ protected function connect() {
       else {
         // Database connection failed for some other reason than the database
-        // not existing.
+        // non-existent.
         $this->fail(t('Failed to connect to your database server. The server reports the following message: %error.<ul><li>Is the database server running?</li><li>Does the database exist, and have you entered the correct database name?</li><li>Have you entered the correct username and password?</li><li>Have you entered the correct database hostname and port number?</li></ul>', ['%error' => $e->getMessage()]));

'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.

jungle’s picture

Status: Needs work » Needs review
FileSize
3.11 KB
12.61 KB

Thanks @catch!

Replaced with what you suggested "Database connection failed for some other reason than a non-existent database." in 3 occurrences/files.

jungle’s picture

Status: Needs review » Reviewed & tested by the community

Setting back to RTBC. Apologize if this is counting as self-RTBC and set back to NR, please.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 43: 3154909-43.patch, failed testing. View results

jungle’s picture

Status: Needs work » Reviewed & tested by the community

Re-queued, random fail.

larowlan’s picture

Category: Bug report » Task

I think task is the right category here

  • catch committed b62e284 on 9.1.x
    Issue #3154909 by jungle, adityasingh, nitesh624, mohrerao, rajandro,...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed b62e284 and pushed to 9.1.x. Thanks!

Status: Fixed » Closed (fixed)

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