Docvert breaking _config.php execution

silverstripe / cwp-core

CWP basic compatibility module

BSD 3-Clause "New" or "Revised" License

3 stars 12 forks source link

Docvert breaking _config.php execution #56

Closed brettt89 closed 5 years ago

brettt89 commented 5 years ago

https://github.com/silverstripe/cwp-core/blob/1e8af7a07d0f3aeab2685ad268e1b9d35c3d20a4/_config.php#L19

When running Docvert on CWP, the Manifest fails to properly include this extension if Page class does not exist. This should be using BasePage or SiteTree as these classes exists within the cwp module that is required by this one.

E.g.

BasePage::add_extension('DocumentConverterDecorator');

NightJar commented 5 years ago

🤔 Hmm... it's a tricky one - Page is assumed to always exist throughout many a module... and does exist in CWP too. However it's a project level class, as opposed to module level, and thus can always be altered.

cwp/cwp-core in itself does not add any project related code, so perhaps it makes more sense to properly extract docvert to cwp/cwp where it can then be added to BasePage.

On closer inspection, I cannot find this in the most recent versions. It may be that the version you're finding this on is out of support. Can you please report back with the version @brettt89 ?

brettt89 commented 5 years ago

Yeah I think it makes more sense to be in cwp/cwp as you have suggested.

This is an issue for version 1.9.

This is why it is an issue for a specific codebase.

Codebase X uses Traits in Page class
They have a custom Trait loader that loads traits into the manifest using _config.php.
SilverStripe loads manifest classes alphabetically.
cwp/cwp-core is loaded before trait-loader manifest and has a reference to Page class.
Website throws an error as Trait class does not exist in the Manifest yet.
SilverStripe stops loading any further manifests due to this error.

NightJar commented 5 years ago

wow, that is pretty intense! So now comes the tricky part; is it considered a breaking change to move the docvert support from cwp/cwp-core to cwp/cwp? I'm inclined to say yes. But then we could move only the configuration, as that wouldn't shift the dependencies... however I'm not 100% sure that would solve your issue.

From memory project _config.php is always last to load. In the case it's not, it's alphabetical. So you could work around it by renaming the project (e.g. mysite => zzloadmysitelast) configurable via the combination of the folder name, and the global $project value in said php config file.

robbieaverill commented 5 years ago

Sorry, we don't [commercially] support plugins that change the way SilverStripe loads its classes or configuration. Closing as #wontfix