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

dev/core#2823 Move validation into validation function #21400

Merged
merged 1 commit into from
Sep 11, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Sep 8, 2021

Overview

dev/core#2823 Move validation into validation function

Before

The validate function has a comment about checking the module is recognised - the enable function throws an exception if it isn't

After

The validate function ... validates

Technical Details

As the code comments suggest the handling of a module being unrecognised should
be handled in the validate not the enable function

I find the moduleIndex pretty nasty - all known modules
are indexed into either the TRUE key or the FALSE
key depending on whether they are enabled or not.

I'm focussing on adding getters to access that info for now
& will later change the structure when the code is not interacting
directly with the property

Comments

@civibot
Copy link

civibot bot commented Sep 8, 2021

(Standard links)

foreach (['name', 'module', 'entity', 'params'] as $key) {
if (empty($declare[$key])) {
$str = print_r($declare, TRUE);
return ("Managed Entity is missing field \"$key\": $str");
return ts('Managed Entity (%1) is missing field "%2": %3', [$module, $key, $str]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think these errors are seen often enough to warrant a translation

As the code comments suggest the handling of a module being unrecognised sould
be handled in the validate not the enable function
@colemanw
Copy link
Member

Refactoring makes sense and tests are happy.

@colemanw colemanw merged commit d8538fc into civicrm:master Sep 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants