Deprecate drupal_rewrite_settings()

I don't think this should be a service. I think this should be a utility class and the class should require the path to the settings file to be pass in.

I.e. the class should not do:

    if (!isset($settings_file)) {
      $settings_file = $this->sitePath . '/settings.php';
    }

of have $this->sitePath injected. The constructor should just accept a string and it should be a path to a settings file to rewrite.

Looks good, needs to edit CR and could be rtbc

The trick is to document that argument order is opposite

Thank you, looks ready

Posted some review comments plus there's quite a few mentions of drupal_rewrite_settings() even in new code that should be fixed.

Taking a look at

there's quite a few mentions of drupal_rewrite_settings() even in new code that should be fixed.

per #13

Ah, no I see that has already been handled.

I first came here to provide a review.

I've had a look through the code and see nothing out of place, test coverage looks good and is passing. The CR conveys the necessary information and is up to date.
I would make this RTBC except for the number of times I've seen issues rejected because the issue summary is not up to date, so, tagging for that.

This looks ready to me.

This now fails the new tighter PHPStan rules...

Running PHPStan on changed files.
 ------ -----------------------------------------------------------------------
  Line   lib/Drupal/Core/Site/SettingsEditor.php
 ------ -----------------------------------------------------------------------
  107    Static method Drupal\Core\Site\SettingsEditor::isArrayIndex() invoked
         with 2 parameters, 1 required.
  119    Variable $index might not be defined.
  122    Variable $index might not be defined.
  138    Variable $current might not be defined.
  140    Variable $index might not be defined.

We need to fix the one on line 107 and regenerate the baseline for the others as they will move from install.inc to this file.

Not sure that this commit is quite correct, given it is changing the refactored code but the PHPStan baseline changes refer to the original code?

Oops, something ate my comment...

Thanks for your work on this API improvement!

I left a number of comments on the merge request. I tried to skip past any issues with what are essentially moved/copied lines and focused instead on the high-level naming and structure of the new API.

Thanks! I polished the text of the CR a little, and left a few more suggestions on the MR.

All review points were resolved, this looks good to me.

I was hopeful that the (comparatively speaking) new git option for moved lines would be helpful here, so I tried this locally:
git diff --color-moved --staged -w

Unfortunately, this does not quite have the expected results, e.g., it works as expected for the PHPStan baseline:

But not so much for the actual function:

So, I had to get a bit creative, and put the original and new versions of each method into its own file, and diffed them using git diff --no-index --color-words -w. Using this method, I confirmed that for the most part, the only changes to the individual methods are :

• The method renames.
• The addition of parameter and return typehints.
• The recommended fixes to the one-line method documentation.
• The documented changed order of arguments for ::rewrite().
• Throwing \Exception instead of Exception since it is no longer in the global namespace.

Other changes include:

1. A switch from == to === and != to !== in various methods.
2. The wrapping of the following line:
   

       $is_boolean_or_null = $type === T_STRING && in_array(
         strtoupper($value),
         ['TRUE', 'FALSE', 'NULL']
       );
   

3. A correction to the typehint of the $ref parameter from array|null to mixed
4. This change:
   

   -  if (!in_array($type, [T_WHITESPACE, T_COMMENT, T_DOC_COMMENT])) {
   +  if (!in_array($type, [T_WHITESPACE, T_COMMENT, T_DOC_COMMENT], TRUE)) {
   

5. The removal of an else:
   

   -    else {
          // In case any $settings variables were written, import them into the
          // Settings singleton.
          if (!empty($settings_settings)) {
   @@ -165,8 +174,7 @@ function drupal_rewrite_settings($settings = [], $settings_file = NULL) {
          // in case an opcode cache is enabled.
          OpCodeCache::invalidate(DRUPAL_ROOT . '/' . $settings_file);
        }
   -  }
   

6. This addition:

       * @endcode
   +   * @code
   +   *   'sites/default/settings.php'
   +   * @endcode
   +   * @code
   +   *   \Drupal::getContainer()->getParameter('site.path') . '/settings.php'
   +   * @endcode
   +   *
   +   * @throws \Exception
       */
   

Only that last two changes seemed off.

Looking at the code for #5, the if for that else throws an exception, so the code would not run anyway if the condition were met. So that is a good cleanup too.

However, I don't see the purpose/value in the lattermost addition and I could not find it in the original code. It also led to me noticing that the docblock is out of order; all the how-to-use-it docs should come above the @param.

I pushed a couple commits to address #33.6.

Alright. I considered tagging this for framework manager review, but since @alexpott's earlier review already gave feedback on the architectural aspects of this I'd be concerned about, I think that is sufficient.

There might be further improvements and refactoring that could be made for this code, but I think it's best to scope any further improvement to followups, since it's already hard enough to review the changes as per #33.

The changes I made to the docblock are very minor, and my other suggestions got a +1 from longwave as well as implicitly from @voleger by accepting them, so I'm comfortable committing this.

Committed to 10.1.x, and published the change record. Thanks!