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

Add Event Triggers to content Filter #680

Closed
wants to merge 5 commits into from
Closed

Add Event Triggers to content Filter #680

wants to merge 5 commits into from

Conversation

PxaMMaxP
Copy link

@PxaMMaxP PxaMMaxP commented Dec 1, 2023

In this merge I add important event triggers to improve the consistency in the functionality of PicoCMS. Previously, the onContentPrepared and onContentParsed events were only triggered when a page was called directly.

This limitation led to problems with plugins that rely on these events, especially if the content was processed via the internal Twig filter content. The added event triggers in the content filter ensure that all plugins involved in content creation work smoothly even when the content filter is used.

This change ensures broader and more reliable plugin compatibility within PicoCMS.

Copy link
Collaborator

@PhrozenByte PhrozenByte left a comment

Choose a reason for hiding this comment

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

Thank you for your PR! 👍

I assume you accidentally selected the wrong branch to merge, you probably rather meant to select your content-filter-trigger-events branch instead of your pico-3.0 branch. You might wanna open another PR.

Besides, I'm afraid that we can't merge it the way it is, because the onContentPrepared and onContentParsed events were designed to only fire for the requested page, not for other pages as well. This will likely break a lot of plugins, they just don't expect that. For other pages we have the onSinglePageLoading, onSinglePageContent, and onSinglePageLoaded events. Since we indeed don't have events that fire before and after Markdown processing, we could indeed add such new events, just with a different name.

We'd have to let PicoDeprecated know about these new events as well.

@PxaMMaxP
Copy link
Author

PxaMMaxP commented Dec 1, 2023

Heyy,

thanks for your detailed explanations, and sorry for the confusion with the wrong branch.

About the change of the content-filter-trigger-events branch:
My problem was that when I rendered the content of several pages with the content-filter on one page, the corresponding plugins were not applied to that content.

In my specific case, it was the targetBlank plugin. Links were not changed accordingly on the content-->content-filter-->output path.

I noticed that there was no note (issue..) about this and it should have made me suspicious.

I'll close the pull request and take another close look at the whole thing.

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.

2 participants