Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Sagar Ramgade created an issue. See original summary.

Status: Needs review » Needs work

The last submitted patch, drupal-double_occurences_preposition_fix-D8.patch, failed testing.

cilefen’s picture

Issue tags: +Novice

It is amazing how many there are.

Sagar Ramgade’s picture

Status: Needs work » Needs review
FileSize
23.85 KB

Patch re-rolled.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks! Most of these fixes are good. A few notes:

  1. Some of the fixes here are also in the patch you made on #2589733: LocalTaskInterface, XssTest, RendererTest - double 'are'. Either close that other issue as a duplicate of this one, or remove the parts that are on the other issue's patch.
  2. +++ b/core/lib/Drupal/Component/Diff/Engine/DiffEngine.php
    @@ -19,7 +19,7 @@
    - * Some ideas are (and a bit of code) are from from analyze.c, from GNU
    + * Some ideas are (and a bit of code) are from analyze.c, from GNU
    

    You could also fix the double "are" in this line: Some ideas are (...) are from...

  3. +++ b/core/lib/Drupal/Core/Config/StorageComparerInterface.php
    @@ -109,7 +109,7 @@ public function moveRenameToUpdate($rename, $collection = StorageInterface::DEFA
    -   *   'old_name' and and 'new_name' representing the old and name configuration
    +   *   'old_name' and 'new_name' representing the old and name configuration
        *   object names during a rename operation.
        *
    

    This needs to say "... representing the old and new configuration..."

  4. +++ b/core/lib/Drupal/Core/Render/Element/Details.php
    @@ -13,7 +13,7 @@
    + * outside of forms.  Users click on the title to open or close the details
    

    You could also take out the extra space after . in this line.

  5. +++ b/core/modules/simpletest/simpletest.js
    @@ -83,7 +83,7 @@
    -   *   Attaches the filter behavior the the text input element.
    +   *   Attaches the filter behavior the text input element.
    

    Actually, the first "the" should be "to".

  6. +++ b/core/modules/views/src/Plugin/views/pager/SqlBase.php
    @@ -140,7 +140,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
    -      '#title' => $this->t('Allow user to to display all items'),
    +      '#title' => $this->t('Allow user to display all items'),
    

    Note that this is a string change. However, it should be fixed.

  7. +++ b/sites/default/default.settings.php
    --- a/vendor/easyrdf/easyrdf/lib/EasyRdf/GraphStore.php
    +++ b/vendor/easyrdf/easyrdf/lib/EasyRdf/GraphStore.php
    

    Do not change anything in /vendor. The rest of the patch file, all the stuff in /vendor, should be left alone. This is not Drupal project code.

sdstyles’s picture

Status: Needs work » Needs review
FileSize
17.82 KB
GoZ’s picture

I add interdiff between #4 and #6 and remove vendor part.

Patch miss @jhodgdon 1. :

Some of the fixes here are also in the patch you made on #2589733: LocalTaskInterface, XssTest, RendererTest - double 'are'. Either close that other issue as a duplicate of this one, or remove the parts that are on the other issue's patch.

So i remove the 3 fixs of #2589733 which already is RTBC.

Sagar Ramgade’s picture

The second point makes more sense by keeping the latter "are", attached patch resolves that.

+++ b/core/lib/Drupal/Component/Diff/Engine/DiffEngine.php
@@ -19,7 +19,7 @@
- * Some ideas are (and a bit of code) are from from analyze.c, from GNU
+ * Some ideas (and a bit of code) are from analyze.c, from GNU
GoZ’s picture

@Sagar Ramgade looks good, but can you add interdiff please ?

Sagar Ramgade’s picture

FileSize
618 bytes

yes here is the interdiff of #7.

jhodgdon’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Thanks! Patch looks good now.

+++ b/core/modules/views/src/Plugin/views/pager/SqlBase.php
@@ -140,7 +140,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
-      '#title' => $this->t('Allow user to to display all items'),
+      '#title' => $this->t('Allow user to display all items'),

Just noting again that this is a UI string change. However, the existing string has this "to to" typo in it. Should be changed.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +rc eligible

Amazing how often we do this. I agree with @jhodgdon that the UI string should be fixed. This is eligible for RC because it is a documentation change. Committed 93088f8 and pushed to 8.0.x. Thanks!

  • alexpott committed 93088f8 on 8.0.x
    Issue #2590059 by Sagar Ramgade, GoZ, sdstyles, jhodgdon: Fix double...

Status: Fixed » Closed (fixed)

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