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

Make LeftAndMain independent of the presence of Versioned module #1050

Open
wants to merge 2 commits into
base: 1
Choose a base branch
from

Conversation

alessandromarotta
Copy link

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.

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
Copy link
Author

alessandromarotta commented Jun 5, 2020

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

indentation correction for travis
@emteknetnz
Copy link
Member

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.

@emteknetnz emteknetnz closed this Jun 29, 2020
@alessandromarotta
Copy link
Author

alessandromarotta commented Jun 29, 2020

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
Copy link
Member

emteknetnz commented Jun 29, 2020

Hi @alessandromarotta

Versioned in a few different places throughout the admin module

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
Copy link
Contributor

dhensby commented Jun 29, 2020

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
Copy link
Author

alessandromarotta commented Jun 29, 2020

@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
Copy link
Contributor

dhensby commented Jun 29, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants