Make LeftAndMain independent of the presence of Versioned module

[alessandromarotta]

LeftAndMain's init() method calls two Versioned methods and this makes it impossible to have the Silverstripe-admin module installed without the Versioned module.

In other parts of the Silverstripe-admin code we have the verification of the presence or absence of the Versioned class, instead in the init() method it is missing.

Please, could you also introduce in the init() method the verification of the presence of the Versioned module in order to install the administrative panel without the Versioned module?

Like in the file commit, for example.

[alessandromarotta]

p.s: I think that it is better to keep the Versioned module dependece in the composer.json. If someone wants to delete the Versioned module from their installation of the Silverstripe-admin, it could be put in the documentation that can be deleted by placing

"replace": { "silverstripe/versioned": "*" }

in the root project composer.json

[emteknetnz]

The versioned module in a requirement of the admin module
https://github.com/silverstripe/silverstripe-admin/blob/1/composer.json#L23 so it is assumed that it will always be there

Removing the versioned module via "replace": { "silverstripe/versioned": "*" } is not something we actively support

There are various other places in the within the admin module that reference the Versioned class, without first testing to see if that Versioned module exists, because it's always assume that it does

We'll close this pull-request now. Feel free to open a new pull-request for anything else you think could be fixed or improved.

[alessandromarotta]

Hi, have you read here?

"LeftAndMain's init() method calls two Versioned methods and this makes it impossible to have the Silverstripe-admin module installed without the Versioned module.

In other parts of the Silverstripe-admin code we have the verification of the presence or absence of the Versioned class, instead in the init() method it is missing."

I think that it's possible to have the SilverStripe Admin module without the Versioned module.

Only this modification is needed: https://github.com/silverstripe/silverstripe-admin/pull/1050/files

The panel Admin modul should be independent. To add versioning support in SilverStripe Admin this module should be installed https://github.com/silverstripe/silverstripe-versioned-admin

[emteknetnz]

Hi @alessandromarotta

Versioned in a few different places throughout the admin module

• https://github.com/silverstripe/silverstripe-admin/blob/1/code/Forms/UsedOnTable.php#L110
• https://github.com/silverstripe/silverstripe-admin/blob/1/code/CMSBatchAction.php#L168
• https://github.com/silverstripe/silverstripe-admin/blob/1/code/CMSBatchAction.php#L168

These are also not wrapped in class_exists()

We could in theory wrap everything in class_exists(), however then we'd run into the issue where versioned is already in composer.json, and removing it would be breaking change as far as semver is concerned. Therefore such a change would would need to happen in Silverstripe 5.

Given that the versioned module is a requirement in composer.json, class_exists(Versioned::class) is basically just redundant code. As mentioned before "replace": { "silverstripe/versioned": "*" } is not something we actively support. If we were to actively support this then we'd want to be consistent about that for all modules, and that would require us to wrap every call to a different module in class_exists() which would be some pretty over-the-top defensive programming :-)

I hope this clarifies things

[dhensby]

For clarification: Adding and removing dependencies is not a semver issue; semver is about a guarantee of our APIs, not our dependencies. If someone is using admin and they themselves need versioned it should be in their composer.json.

However, trying to decouple versioned from admin is a good idea in principle, but it needs a lot more consideration than just wrapping one use of it in a class_exists call.

[alessandromarotta]

@dhensby ok, but my commit does not break anything, but it would give the possibility, for those who needs, to install the SilverStripe Admin module without the Versioned module, simply adding "replace": {"silverstripe / versioned": "*"} in the composer .json of the project.

I emphasize that in other parts of SilverStripe Admin the presence of the Versioned module is verified with the method has_extension(), less than in the Init() method of the LeftAndMain.php file. This really doesn't make sense.

[dhensby]

as @emteknetnz has pointed out, this is an incomplete implementation as Versioned is used in other places in the code. If you want to go and fix that up I'd be happy to take in a change that made Versioned optional and removed it from composer.json - but we also need to make sure that the other aspects of the module (frontend aspects) aren't making assumptions about versioned either.

---

edit: Ah, sorry, those other parts do check for the presence of versioned - ok. Well, then this should probably be reopened.