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

NEW: Extension points for FilesystemPublisher. #137

Conversation

mfendeksilverstripe
Copy link
Collaborator

@mfendeksilverstripe mfendeksilverstripe commented May 12, 2022

NEW: Extension points for FilesystemPublisher.

  • Extension points added to generatePageResponse() to allow better extensibility

@dhensby
Copy link
Contributor

dhensby commented May 12, 2022

The idea behind the publisher classes is that developers can easily implement their own and add their own logic easily. There shouldn't really be a need for extension hooks with the current implementation.

What is the scenario where this is needed?

@mfendeksilverstripe
Copy link
Collaborator Author

@dhensby My team found useful to have the ability to inject some custom behaviour via these extension points. For example we have in-memory cache which needs to be flushed after each call of generatePageResponse(). It's not something that warrants the implementation of a new file system publisher as we're not altering any core behaviour but instead a simple extension can accomodate this requirement.

Also, we found useful to reset some core states such as controller stack something like this:

try {
    $controllers = [];

    // pop all current application's controllers to simulate cold boot
    while (Controller::has_curr()) {
        $controller = Controller::curr();
        $controllers[]= $controller;
        $controller->popCurrent();
    }

    return parent::generatePageResponse($url);
} finally {
    // flush all in-memory cached data

    foreach (array_reverse($controllers) as $controller) {
        // reapply previous controllers
        $controller->pushCurrent();
    }
}

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your use case seems sensible to me. Please retarget this PR to the 6 branch and remove the unrelated changes.

src/Publisher/FilesystemPublisher.php Outdated Show resolved Hide resolved
src/Publisher/FilesystemPublisher.php Outdated Show resolved Hide resolved
@mfendeksilverstripe mfendeksilverstripe force-pushed the feature/publisher-extensibility branch from d560f65 to b8f60e3 Compare December 22, 2023 00:27
@mfendeksilverstripe mfendeksilverstripe changed the base branch from master to 6 December 22, 2023 00:28
@mfendeksilverstripe
Copy link
Collaborator Author

I've pushed up changes as per your request @GuySartorelli , please review.

@GuySartorelli
Copy link
Member

You swapped the target branch after resetting commits, which means the new CI isn't able to run. Can you please force push so that the CI runs correctly?

@mfendeksilverstripe mfendeksilverstripe force-pushed the feature/publisher-extensibility branch from b8f60e3 to 50519cd Compare December 22, 2023 00:32
@mfendeksilverstripe
Copy link
Collaborator Author

Done @GuySartorelli

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me, will merge when CI goes green.

@GuySartorelli GuySartorelli merged commit d7dcffe into silverstripe:6 Dec 22, 2023
9 checks passed
@mfendeksilverstripe mfendeksilverstripe deleted the feature/publisher-extensibility branch December 22, 2023 00:37
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