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

Reimplement Matcher with plugin instances support #14

Merged
merged 4 commits into from
Aug 13, 2015

Conversation

Berdir
Copy link
Member

@Berdir Berdir commented Aug 4, 2015

PR for #12

Can't comment on the diff, so opening the PR for that :)

@@ -0,0 +1,9 @@
langcode: en
Copy link
Member Author

Choose a reason for hiding this comment

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

Is it really useful to still have that default module? Maybe we should just merge everything into the match module?

I don't know crm_core very well. Those three types are just default config too I guess, there could be others. Do they really still make sense to have, especially without actual configuration?

Copy link
Member Author

Choose a reason for hiding this comment

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

Looking at what's left in the default module:

  • crm_core_default_matching_engine.permissions.yml sounds like it can be dropped anyway.
  • crm_core_default_matching_engine.module is all dead code too I think.
  • tests/src/Unit/Plugin/crm_core_match/engine is in a really weird place anyway :)

So other than the field match plugins/manager, nothing left. definitely seems like a good idea to just drop that module to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Default configuration is used by Collect (demo), not sure either does it need to be removed.

Dropped the crm_core_mdefault_matching_engine module.

All useful code was moved to crm_core_match (including ReadMe.txt which I appended to the crm_core_match read me file.

@Berdir
Copy link
Member Author

Berdir commented Aug 4, 2015

Nice work!

@mbovan mbovan force-pushed the feature/crm-core-arch-changes branch from 21b3560 to 195e064 Compare August 11, 2015 13:37
@mbovan
Copy link
Contributor

mbovan commented Aug 11, 2015

Pushed the latest changes. Most of the @Berdir's suggestions are applied. Some part of them are skipped and set as todo's for followups. I added replies on the feedback comments. They are hidden now as diff is outdated. :D

Patches to apply for Collect and Inmail

Regarding tests, FieldMatcherTest is pasing, fixed some parts and commented out most of the code in DefaultEngineTest and MatcherTest classes. We have to see what we can use/need from those tests...

Followups: #17, #18

@mirodietiker
Copy link
Member

Did a manual test...

After saving a matcher, the system states "The configuration has been saved." without any reference to the matcher saved. But i'm still on the matcher add form... So either a redirect is missing to the edit form, or to the overview form.

The threshold value (required) is not saved.
I don't know why, but i can only enable the field name (string) to match. All others are disabled.

Other than creating a matcher, i don't know how i can test a matcher. Did you already provide the collect integration? I like to test it.

@mbovan
Copy link
Contributor

mbovan commented Aug 13, 2015

@mirodietiker:

  1. Added redirection to the matcher overview page.
  2. Fixed threshold.
  3. I think because String is the only field handler that is not broken. That is the same in 8.0.x branch, so I didn't make any fixes there in this issue.
  4. There are patches for Collect/Inmail in my previous comment that should be applied. Then, you can enable collect_demo.

@mbovan
Copy link
Contributor

mbovan commented Aug 13, 2015

  • Added fix for saving matcher configuration.
  • Add/Edit Matcher form redirects on the same page.

Updated Collect patch (loading a matcher from configuration instead of hard-coding to inmail_individual) with these changes and changes made in Remove event-based processing issue:

mirodietiker added a commit that referenced this pull request Aug 13, 2015
Reimplement Matcher with plugin instances support
@mirodietiker mirodietiker merged commit 4c98f30 into 8.x-0.x Aug 13, 2015
@mirodietiker mirodietiker deleted the feature/crm-core-arch-changes branch August 13, 2015 13:12
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