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

Load contact plugins in the whole contact component #13362

Closed
wants to merge 2 commits into from

Conversation

bembelimen
Copy link
Contributor

Summary of Changes

At the moment you can create a contact plugin to handle the email sending by yourself. But this function makes (mostly) only sense if you add new fields. Adding new fields is only possible with content/system plugins, so you need at least two plugins or a system plugin. This makes the contact plugin a bit useless.

This patch loads the contact plugins within the whole component, so a contact plugin can adjust the form and handle the email stuff.

Testing Instructions

Here you need a contact plugin which e.g. change the form. Before: form is not changed, after: form is changed.

@mbabker
Copy link
Contributor

mbabker commented Dec 24, 2016

Plugin groups are generally imported near where events get dispatched so this should be updated to do the same (yes, I realize that means the call is required in more places, but that prevents unneeded loading).

@mbabker
Copy link
Contributor

mbabker commented Dec 24, 2016

The other thing about it, while I'm thinking of it, is if you're using the models to do something (like saving a contact when another action happens), the model should be importing the required groups before it triggers events. With it this way, that wouldn't happen.

@bembelimen
Copy link
Contributor Author

A completly other idea: perhaps we should deprecate and remove the contact group, because at the moment you can perform all the task, which add this patch, with a normal content plugin...so a contact group does not make any real sense here?

@mbabker
Copy link
Contributor

mbabker commented Dec 24, 2016

You don't need groups at all, it's just an artificial way to limit how many of the plugins are actually loaded into the system. I'd rather see the concept as a whole phased out in favor of better overall event management and registration, but the couple of people I've brought that up to groaned miserably.

@bembelimen
Copy link
Contributor Author

Yes I know, it just matter, which plugin is loaded...I know, we could throw everything in system plugins and everything would work.

This patch is a try to stay B/C...

@brianteeman
Copy link
Contributor

As we now have custom fields in the core is there really a need for this?

@mbabker
Copy link
Contributor

mbabker commented Sep 9, 2017

As a concept it's fine (there are events that fields aren't used with), the implementation though is wrong as I pointed out previously.

@brianteeman
Copy link
Contributor

I am going to close this. @mbabker points out that the implementation is wrong and clearly after such a long time there is no interest in updating the PR. It can always be reopened if updated

@brianteeman brianteeman closed this May 8, 2018
@bembelimen bembelimen deleted the contact branch August 13, 2018 02:31
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.

None yet

5 participants