Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v9 - Proposal to ensure addons are compatible with v9 before updating a site or installing addon #9746

Closed
JohntheFish opened this issue Aug 19, 2021 · 11 comments
Labels
Product Areas:Upgrading Status:Proposal This needs discussion before anyone should start coding. Type:Enhancement A need for something new.

Comments

@JohntheFish
Copy link
Contributor

JohntheFish commented Aug 19, 2021

My experience of running v8 addons on v9rc sites so far has been mixed.

Simple blocks can look OK on the page, but the block edit dialogs usually need some work to look good because form controls, floats, bootstrap labels and font awesome icons have all changed.

More complex edit dialogs, dashboard pages and code that integrates with other core functionality need a lot of work. Not many major changes, but a big accumulation of all the time tracking down many little changes and testing.

Many sites owners will update from v8 to v9 without thoroughly checking addon compatibility first, then either discover that dialogs etc are seriously glitched or brick their site. We get a rush of ‘failed update’ forum posts on every major core update and I can foresee v9 being a record breaker.

Many v9 fresh install site owners will also try and install addons/themes that have not been updated for addon compatibility and consequently glitch or brick their site.

My proposed solution is to have a flag in package controllers confirming maximum version compatibility. If not set, it will default to the previous major core version.

Package install and the core upgrade procedure in v9 can then check that flag and refuse to update unless all packages are already flagged v9 compatible.

for example


// core base class
abstract class Package implements LocalizablePackageInterface{

   public function getMaxAppVersion(){
      if(isset($this->maxAppVersion)){
         return $this->maxAppVersion;
      } else {
         // sets a default of v8.latest that prevents any un-confirmed v8 package being installed on v9
         // max handles special case of v7 addons already in use on v8 sites
         return max( 8, extract_major_version_from($this->appVersionRequired ));
      }
   }
}

// An updated package
class Controller extends Package
{
    protected $appVersionRequired = '8.5.2';
    // This package has been updated by the developer and confirmed as v9 compatible,  but will not be compatible with v10.
    protected $maxAppVersion  =  '9';
    //....
}

// A package that has not been updated
class Controller extends Package
{
    protected $appVersionRequired = '8.5.2';
    // This package has not been updated. It defaults to only being compatible with v8
    //....
}

// A package built for v9
class Controller extends Package
{
    protected $appVersionRequired = '9.0.0';
    // This package has been built for v9. It is automatically compatible with all v9, but will not be compatible with v10.
    //....
}

This flag can also be used in the marketplace to provide a big 'v9 Compatible' rosette on updated packages.
The mechanism is automatically capable of handling further major version updates.

A beneficial side effect of this flag will be to weed out addons that are not actively maintained in the marketplace.

@JohntheFish
Copy link
Contributor Author

I have asked a question about this for the September Town Hall
https://concrete5.slack.com/archives/CDX2TLZ6E/p1630161580002100

@JohntheFish
Copy link
Contributor Author

My question missed the town hall. Some discussion on slack when I posted
https://concrete5.slack.com/archives/CDX2TLZ6E/p1630161580002100

@JohntheFish
Copy link
Contributor Author

@aembler
Copy link
Member

aembler commented Sep 21, 2021

This is something that should be rolled into the updater.

Screen Shot 2021-08-24 at 8 17 05 AM

In case you're not familiar, Concrete has a built-in update tool. If connected to the marketplace, it is capable of scanning your add-ons and presenting compatibility warnings. The only issue with this in my mind is the updater currently only works off of packages that are actually available locally to update to. This doesn't work if you haven't downloaded the update yet to update to, and it also isn't an option if you're using something like composer to update to.

I think the update checker functionality and the actual "run the update now!" functionality should be separate. You should be able to check your current site's compatibility against an upcoming update without having to actually try and update the code base, or use the Concrete built-in updating functionality to update.

@aembler aembler added Product Areas:Upgrading Status:Proposal This needs discussion before anyone should start coding. Type:Enhancement A need for something new. labels Sep 21, 2021
@JohntheFish
Copy link
Contributor Author

That only works if developers edit the multiple selects for marketplace compatibility to specify if they are (or are not) compatible with a version.

The mechanism has become a sprawling and unusable mess. Developers don't maintain these selects because there are better things to waste their life on. If you have an addon released with 5.7.0 and specify a version compatibility on that marketplace page, you then have a plethora of selects that have to be to set individually for every single minor version up to latest and set again every time you update an addon.

Hence no-one uses that mechanism because it is horrendously unusable.

Also, it doesn't work if a site is not connected to the marketplace.

The proposal here is to have a property in a package controller that specifies maximum version compatibility and defaults to the core major version or v8, whichever is greater. This protects any backward compatibility with core versions in the future and has no dependance on marketplace connection.

Unless a developer updates a package for v9 and sets the property to 9, then the core updater can refuse to update the core.
After the core is updated to v9, the dashboard extend/install can omit anything not v9 compatible and refuse to install incompatible packages.

That way:

  • Any site owner clicking 'update' for a v8 site to update to v9 will not end up with a broken site.
  • The dashboard extend page can refuse to install incompatible packages.

If we don't implement such a mechanism, we will have a whole marketplace of addons and themes that default to compatibility with the latest version (v9). The vast majority of v8 sites updating to v9 will then break because an addon has not been updated to work with v9.

@aembler
Copy link
Member

aembler commented Sep 21, 2021

Well, you seem to have strong opinions about this. Clearly my response to this proposal isn't something you're interested in discussing so I'm going to let others continue to this.

@JohntheFish
Copy link
Contributor Author

My apologies for appearing heavy handed. That was not my intent. I do like the prospect of being able to check compatibility before attempting and update. But I don't think such a check can rely on developers maintaining 'Map Files to Compatible Versions'.

  • 'Map Files to Compatible Versions' has become unworkable and is avoided by developers. With v9 the information in that section will default to 'compatible' for many marketplace items that have not even been updated to work with v8.
  • Experience of updating for v9 by myself and other regular marketplace developers is that the nearly all packages will need at least some changes in order to avoid breaking either the addon or the site. The changes required are rarely just cosmetic to the edit dialogue.

Whatever mechanism we end up with, the key requirement is that a package developer needs to make a positive assertion that a package is compatible with v9. All packages ignored by their developers, abandoned or incompatible, would then be excluded from v9 by default.

Implementing $maxAppVersion and requiring it to be set for v9 (defaulting to v8.* where not set) would achieve that requirement while coexisting with existing mechanisms.

@aembler
Copy link
Member

aembler commented Sep 24, 2021

I understand the effort behind this, but I just don't think this is a workable approach. We've already cut off backward compatibility for add-ons from 5.6 to 5.7, and you're asking me to do it again. I understand that it's a different situation because the developer can just mark the add-on as compatible (whereas with the 5.7 shift there was obviously no way to do that without rewriting the add-on) but by default the add-on is what, unavailable?

I also don't really understand how this will help in a practical sense. Let's say a developer doesn't keep up with the "compatible versions" checkbox on the marketplace. So their add-on is then served by default to someone buying it for v9. What happens when they download it and the add-on code itself doesn't let them install it? That sounds like a nightmare.

I'm not willing to make the assumption that there is a huge population of wholesale broken add-ons out there. Will the updates in dependencies break some more complex add-ons? Absolutely, and that's not ideal, but it's not something we can really help given the way we're hooked into some external dependencies. Are there simpler add-ons that are also broken in v9? If that's the case, I'd rather figure out some simple ways to help those add-ons not be broken, if at all possible. I'm also exploring better backward compatibility for themes to ensure that they are not broken post-update.

If there is indeed a huge population of broken add-ons, rather than just disallowing all add-ons that aren't flagged as compatible, I'd much rather do the following.

  1. When v9 is added to the version list, do NOT mark existing add-ons as compatible by default. That means v9 shows up in the marketplace version selector, but add-ons are not just opted in as compatible with it.
  2. Do not change the way add-ons actually install in Concrete.

What would this do? It would disallow developers from selling for version 9 unless they specifically opted in. This would force them to vet the add-on.

By the time a user actually has an add-on on their site and is trying to install it, I don't see how a version check at that time is going to help them, unless it also matches the compatibility of the versions listed on marketplace.concretecms.com

@JohntheFish
Copy link
Contributor Author

JohntheFish commented Sep 24, 2021

I think your note (1) would be workable. I realise marketplace integration from a site checks the version list for package install/update, but does the core updater also check that before updating?

We have 2 site-breaking scenarios:

  1. A v8 site with addons installed updates to a v9 core and breaks because of incompatible addons - only be recoverable from a backup
  2. A v9 site then installs an addon that is not compatible with v9 - usually recoverable by uninstalling

When v9 is released, I foresee (1) as being the big risk. Once v9 has been around for a while, the risk of (1) will diminish and (2) will grow as new sites install addons.

When implementing, please consider the long term process. Developers shouldn't need to reassert v9 compatibility with every subsequent addon update or every subsequent minor core version. Once v9 compatibility has been confirmed, when a developer subsequently uploads a new addon version it should automatically apply to all listed core versions (the current default behaviour unless a developer has decided to set version compatibility manually). If a developer maintains a single addon code base across 50 core versions, they shouldn't have to set 50 selects each time they update their addon.

When you are working on the code in that section of the marketplace, anything you can do to improve the usability of managing the long list of selects against core versions will be good for long-term developers. Perhaps a *latest* option? Perhaps a button to set all? Perhaps changing it to something other than a long list of core versions? I don't know what is feasible with the underlying data.

By the time a user actually has an add-on on their site and is trying to install it, I don't see how a version check at that time is going to help them, unless it also matches the compatibility of the versions listed on marketplace.concretecms.com

With that in mind, what is the difference between the proposed $maxAppVersion and the current $appVersionRequired property?
https://github.com/concrete5/concrete5/blob/c8d75f8b2b2ff3bdf06b088eeff2dfac2a86605a/concrete/src/Package/Package.php#L797

Isn't $appVersionRequired also parsed by the marketplace uploader?

Historically, it has always been much easier for those outside of Portland Labs to get a change made to the cms core than it has been to get Portland Labs to make a change to the marketplace site. Hence my enthusiasm for $maxAppVersion and mechanisms divorced from the marketplace site. It enables everyone to get involved in using and enhancing.

@linuxoid
Copy link

I'm afraid while you concentrate on a technical side of the issue, my original concern has been lost in the technicalities.

I believe there should be a warning in the Market Place saying that (some) v8 packages may not work in v9 out of the box unless clearly stated otherwise to avoid customers paying for a package, realizing it doesn’t work or looks ugly and then demanding a refund thus getting the package for free. Or a separate section should be made for v9 packages in the MP.

I’ve just checked a couple of my big applications in v9. Well, they’re totally screwed up and ugly as hell. So although versioning is good, who knows when or if it will be implemented. But I think the warning must be put in the MP now regardless.

Or do I have to edit all my package pages and put the warning myself that they don't work in v9?

@aembler
Copy link
Member

aembler commented Feb 16, 2022

We have since placed a warning in the marketplace and I consider this resolved.

@aembler aembler closed this as completed Feb 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Product Areas:Upgrading Status:Proposal This needs discussion before anyone should start coding. Type:Enhancement A need for something new.
Projects
None yet
Development

No branches or pull requests

3 participants