[policy + docs] Decide on backwards compatibility policy for Composer plugins in Drupal 8

The other thing we need to be careful of is that if the Plugin class instantiates objects (or, more specifically, loads classes) in the `activate` method, and those classes are updated as part of a `composer update` operation, then the old version of the code loaded in `activate` will continue to be used in the post-update hooks, whereas in contrast, classes loaded after the update will be loaded from the new code.

We should investigate avoiding this problem by not loading any classes (other than the Plugin class) in the activate method. Once we have proven this technique, we should document it as part of the bc policy / strategy for Composer plugins.

Yeah, we already use a handler pattern; it was exactly my thought that we should lazy-instantiate the handler.

Lazy-instantiation of the handler done for scaffold plugin in #3104922: Guard against changes to Scaffold Plugin that might cause upgrade problems and committed to 8.9.x and 9.0.x. Still needs backport to 8.8.x.

I updated the issue summary to include proposed text for the new policies.

Perhaps we should go one step farther and propose that command line tools and scripts may include new features in patch releases. Do we know if the "no new features in patch releases" policy has been rigorously enforced for scripts in the past? If this policy change is at all controversial, we could discuss it in a follow-on issue (or simply drop the question if deprecated).

I guess this should be "needs review" now.

Just to be explicit, I agree with #9. Let's keep the proposal as presently written, and adjust the policy for a command line tool framework if/when such a thing exists.

Updated the issue summary to include information about backwards compatibility policies for configuration elements.

Add a patch marking all Composer Plugin classes @internal.

There is a patch now, so adjust title to reflect that.

Slack conversation from yesterday:

@alexpott
My gut feeling is that the config should be the “API” layer. And as long as existing config works as expected then we’re good.
I don't think these plugins are extension points

@larowlan
that seems reasonable

@webchick
It's hard for me to parse out "impact" from that issue summary...
"BC" in the context of a Drupal release means that if you wrote code that was compatible with Drupal 9 for the 9.0.0 release, it can sit there stale and untouched until 9.40.2 and you're fine.
(we have historically had mixed results with that promise, but that's the idea behind it :P)
So if these are internal changes only that only impact how Drupal itself does Composer-y things under the hood, and no contrib/custom code would be affected, :+1:
If we suddenly introduce new requirements, like all modules now need a composer.json in their root, or all sites must now use a completely different directory structure, we'd need to jack the major release number for that, IMO.

@greg.1.anderson
The b/c policy for Composer plugins are fairly removed from any requirements for modules. The scaffold plugin introduces config that install profiles might use to place files, but that's config, not APIs in the usual sense.
It seems there is general agreement about this (the "API" definition for Composer plugins). How do we get the policy issue "merged" (moved to "fixed" with documentation updated as suggested on the issue)?

@alexpott
I think we should clearly state that a composer.json file that uses these plugins in 8.8.0 is expected to work in Drupal 9.0.0
And if we introduce any changes to configuration then we will do in BC compatible manor ie. by deprecating and providing a path to move to an undeprecated config

@greg.1.anderson
@alexpott Do you think that policy should be stated on https://www.drupal.org/core/d8-bc-policy, or somewhere else?

@alexpott
@greg.1.anderson I think this issue should propose an edit to that page. And then it is up to the committers - espically the release managers to approve the changes to that page.

@greg.1.anderson
Yes, it currently does suggest an edit to the page. I can enhance it to also talk about composer.json configuration as suggested above, although the page does not currently discuss things like that, so I'd probably have to propose a new section heading.

@catch
So +1 to making sure 8.8 composer.json is supported.
i think I'm +1 to making the actual plugin code @internal bug why not explicitly mark it all @internal when we add it?

@alexpott
We should definitely get in the habit of adding new things like this as @internal

@greg.1.anderson
Should I make a new issue that adds @internal to all of the Composer plugin classes? Is it okay to just put @internal at the top of the class file, or does every public method need to be @internal? Protected methods too?

@alexpott
I think if the class is @internal then all the methods are
Also I think the patch could be in this issue because if we sign off on this approach then all the classes are @internal

@greg.1.anderson
OK sounds good, I will just add the code patch to the documentation issue then.

@Mile23
+1 on this conversation.

@catch
Also final?

Here is an updated patch with 'final' attached to each internal class.

Exceptions:

• I skipped FileSecurity.php in VendorHardening, because that is a duplicate of a file in core, and I did not want to change the core location in this patch.
• I skipped Config and VendorHardeningPlugin in VendorHardening, and Message in ProjectMessage because marking any of those classes final produces warnings in tests that mock them.

I figure that mocking warnings trump 'final', because classes that are already marked '@internal' should not be used or extended. Today making those classes final only produces a warning, but that warning could become an error in future versions of PHP, introducing technical debt.

Patch does not apply; I'll reroll later.

I feel like final is belts and suspenders. Not terribly opposed to it, but as mentioned in #17, we might need to remove some final labels as time goes on. Most of the classes here are not intended to be extended anyway, so I don't think that concern is to severe. Happy to revert to simply "@internal" though.

Here's a re-roll of #14, which was actually a bad patch. For the interdiff, see #14, as the interdiff was fine.

This one still has the final markers, except where stipulated in #14.

Here is an updated version of the patch in #19, with the final keyword removed. There is no guidance on the use of final in the Drupal Object-Oriented code guidelines; it therefore seems out of scope to stipulate the way that final should be used in plugins vis-a-vis the backwards compatibility policy.

Let's keep the question of whether and how the Drupal coding standards should advise on the use of final a separate issue. If anyone feels strongly about that topic, they may create a follow-on issue against the coding standards.

Here are the patch files that should have been in #20.

#20 / #21 had garbage in it. Also, it just reverts back to #11. So, please ignore all patches after #11, and just review that one.

Re-uploading #11 as #24 for the avoidance of doubt.

I updated the Issue Summary to cover #25:

In the context of a Composer plugin, @internal means that the class or method is internal to that plugin only, and should not be used from other Composer plugins or from any other part Drupal core outside of that plugin.

I updated the Drupal 8 backwards compatibility and internal API policy (backend) page per the issue summary.

Leaving in "needs review" so that a second party can confirm that the updates conformed to the agreement and there were no mistakes made in the transcription. (Please fix mistakes if any are found.) I don't think that we need to ask a core committer to do this verification.

@mradcliffe Thanks for the improvements. Please make minor grammar corrections directly to the documentation page.

Was #37 intended for a different issue? I don't see how #3116405 affects the backwards compatibility policy. If it does, could you please explain? Is the fix for Composer 1.10 a "breaking" change under this policy?