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

FIX Revert adding extension hook #10291

Merged
merged 1 commit into from
Apr 26, 2022

Conversation

emteknetnz
Copy link
Member

@emteknetnz emteknetnz commented Apr 25, 2022

Revert https://github.com/silverstripe/silverstripe-framework/pull/10168/files

This hasn't been tagged, so we can remove the API

Adding this extension hook caused a number of strange issues when running unit tests in CI when there was a module present, namely cwp/cwp, which contains an old fashioned _config.php file that calls Director::absoluteBaseURL()

This ends up causing the manifest to be created twice, which normally wouldn't be a problem, but the manifest cache (an array property on a class) is not respected as there are 2 'surrounding classes' so there ends up being two sets of service classes (%$ references) being created, which is clearly wrong since they are supposed to be singletons.

I don't think this PR was inherently wrong, I think it's more an underlying fragility in the Silverstripe test harness.

@michalkleiner
Copy link
Contributor

Interesting 👀

@GuySartorelli
Copy link
Member

Would you mind creating an issue for the underlying problem here? I have no problem with reverting this change given how minimal it was and how close we are to the next minor release - but I also would be keen to know that this is something we're tracking and will hopefully eventually fix - especially since we don't know exactly what scenario causes this (i.e. is it specifically calls to extension methods on a singleton of Director during _config.php processing? Or on any singleton at any time and we just happened to catch it in this case? etc)

@michalkleiner
Copy link
Contributor

I had a quick look and it seems that none of the issues related to the PR were marked as done or relying on the added hook, so it's possibly ok for now, but it would be great if we didn't lose the hook addition completely, so perhaps open a draft PR with it again and keep it until the config issue is resolved? Or change it from a singleton call to something else to work around the issue?

@emteknetnz
Copy link
Member Author

Have created issue with replication steps - #10292

@GuySartorelli GuySartorelli merged commit 2411a83 into silverstripe:4 Apr 26, 2022
@GuySartorelli GuySartorelli deleted the pulls/4/revert branch April 26, 2022 00:00
@GuySartorelli
Copy link
Member

it would be great if we didn't lose the hook addition completely, so perhaps open a draft PR with it again and keep it until the config issue is resolved? Or change it from a singleton call to something else to work around the issue?

I've created #10293 as a draft PR pending the underlying issue being fixed. I think given the test that breaks is in kitchen-sink rather than in this module directly, it's safer this way than trying to add it in some other way - though, in the long-run, a way to call extension methods statically would be potentially useful for situations like this🤔

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

Successfully merging this pull request may close these issues.

3 participants