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

CRM-20251 Make it easier to overwrite front end form help text regard… #9970

Merged
merged 1 commit into from
Mar 22, 2017

Conversation

eileenmcnaughton
Copy link
Contributor

…ing recurring

@eileenmcnaughton
Copy link
Contributor Author

test this please

@eileenmcnaughton
Copy link
Contributor Author

unrelated fail

@eileenmcnaughton
Copy link
Contributor Author

test this please

@adixon
Copy link
Contributor

adixon commented Mar 13, 2017

Very nice!

@eileenmcnaughton
Copy link
Contributor Author

@adixon is that approval to merge?

@adixon
Copy link
Contributor

adixon commented Mar 13, 2017

Hmm, well, if you put it like that ... how about putting the getRecurringHelpText static function on the contribution object where a payment processor could override it?

@eileenmcnaughton
Copy link
Contributor Author

I don't have a problem with that, but perhaps it should be more like 'getHelpText('recurring, array());

so it can support multiple, I don't really like passing around 'random' arrays, but I suppose the keys for each piece of text could be documented in the docblock.

@adixon
Copy link
Contributor

adixon commented Mar 16, 2017

I think you're proposing a general getHelpText method for the payment processor object, that can be re-used/expanded so that all help text can be customized per payment processor? That sounds excellent. If the method is NOT static, then it wouldn't need the recurring parameter, right? Or are there times where you want help text without a specific contribution? Hmm, I guess that starts getting a bit too icky, I'd go with the single associative array argument with defined keys that can be expanded over time.

@eileenmcnaughton
Copy link
Contributor Author

@adixon what do you think of it now? I admit my distrust of people adding weird & wonderful undocumented things has come through in the way I implemented it .....

Note this can be overridden by the payment processor, but it can also be overridden in a std hook in any extension by assigning a value to the template.

@adixon
Copy link
Contributor

adixon commented Mar 22, 2017

You might regret asking, but I went ahead and have a slightly different version of your getText function version here:
master...adixon:CRM-20251
I toned down your distrust a little bit with an eye to making it more readable and easier to add more contexts in the future and also tried to make the documentation consistent about what kind of strings this function is intended to handle. Somewhere we should document this for payment processors and provide a model for overriding. I was going to change your word 'context' because it's so overused, but I couldn't think of a better one.

@eileenmcnaughton
Copy link
Contributor Author

Dang I tried to switch to yours but it has style errors

@eileenmcnaughton
Copy link
Contributor Author

@adixon ok, your changes are incorporated in here now, and I agree they are improvements, despite my general hatred for 'switch' I agree it is right here

@eileenmcnaughton eileenmcnaughton merged commit 9445a08 into civicrm:master Mar 22, 2017
@eileenmcnaughton eileenmcnaughton deleted the front_end_recur branch March 22, 2017 20:49
monishdeb pushed a commit to monishdeb/civicrm-core that referenced this pull request May 2, 2017
CRM-20251 Make it easier to overwrite front end form help text regard…
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