Guard against changes to Scaffold Plugin that might cause upgrade problems

Here is a patch that helps to guard the Scaffold plugin from future upgrades, and also adds a test to confirm that upgrading won't break.

Running tests is cool.

#2 was missing the new test file, so here is a new patch that contains it.

No interdiff, because the only difference between #2 and #4 is the addition of core/tests/Drupal/Tests/Composer/Plugin/Scaffold/Functional/ScaffoldUpgradeTest.php

1. It would be possible to not cache the handler object. However, it's helpful to keep it as a general technique, and furthermore, throwing away the handler object will not save us from upgrade errors, if there are any. The first time a handler object is created, the Handler.php source file will be included. PHP won't allow us to load the same file more than once, so even if we throw away and recreate the handler, the new handler will still be using the old version of the source code.

2. True. I was concerned about changing the Handler API, as I did not want to introduce a breaking change (since the old versions of the Scaffold plugin instantiate a Handler object early). However, in this case it is possible to change this part of the Handler API without breaking the old Scaffold tool, so I went ahead and did it. The test confirm that this internal API change works.

3. The intention of this call to composer install is just to set up the scaffold plugin before we downgrade and then upgrade it again. We are still testing to see that the Scaffold plugin is working here, but this is more of a control than a test. Updated the comment for clarity.

4. The comments are in the correct order. `composer config --unset repositories.packagist.org` removes the "packagist.org": false from the composer.json file. Removing this configuration allows us to use Packagist again. Similarly, when we no longer want to use Packagist, we re-insert the false configuration.

5. If the operation really didn't work, then the exec will return a non-zero status code, and the test will fail. If the SUT is not being re-installed, then the "Symlinking from" line will definitely be missing. There are many possible messages that will NOT appear when the operation works correctly; I'm not sure that it is necessary to try to enumerate them with a NotRegExp. I'll add any that you think are particularly important, though.

One coding standard message -- fix for that coming right up.

Updated patch.

The plugin class is always instantiated by Composer before any part of the plugin runs (activate in the plugin class is the first thing that runs). Once the plugin class is loaded, it won't change, even if the rest of the plugin code is updated. Because of this dynamic, sometimes the solution is to move more responsibilities into the plugin class.

The main weakness is going to be the contract between the plugin class any any other class that it instantiates before a "post" event (e.g. post-update). Any method of any such class called by the plugin class must be frozen forever. If it is not, users will have to know to run composer update --no-plugins when they upgrade that plugin from the older version to any new version with a new API.

When we analyze the other plugins by this criteria, we see that:

The ProjectMessage plugin only has "post" events, and does not instantiate any objects in its activate method. Unless this changes, this plugin is going to be safe to update. This is also a simple plugin, so it should not have a great need to change in a way that would make future updates dangerous.

The VendorHardening plugin has both "pre" and "post" events. It would be really difficult to avoid instantiating any objects until the "post" events are called. However, the VendorHardening plugin also has very few classes: just a file security class, and a config class. As long as the API for these classes never change, the VendorHardening plugin should also be safe to update moving forward.

The remaining task, then, should be to make a follow-on issue to convert the test here to a build test so that we can have upgrade tests for all of the plugins, to avoid the possibility that someone might unwittingly make an unsafe change to some plugin.

We are expecting that the handler should never be instantiated at this point. $this->handler = NULL would not help; if the handler was instantiated, then the Handler class would already have been autoloaded, which is unrecoverable.

I'll throw an Error here to catch early instantiation of the handler object.

New patch that throws an error when expectations are not met.

Thanks for the review @mile23. Throwing an error in onCommand will hopefully cause things to "break on the bench" and never get to the point of being committed or released to anyone; at least, that's the theory.

Woot! Thanks everyone!

My explanation from a slack convo:

This could still potentially cause upgrade problems, e.g. if someone upgrades straight from 8.8.1 to 9.0.0 (or whatever version changes the handler). Better to make sure the "dangerous" versions are as far in the past as possible.

Thanks for asking about the backport @alexpott.

Updated IS per #26.

Updated the release note snippet in the IS to be more specific about the exact change that was made here.

This will slightly increase the window where we will need to be extremely cautious about changes made to the scaffold plugin. This is probably okay, though, as we have tests that will catch problems. It will just have to be a limitation we live with for a while.