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

Move towards standardising website.create function. #11694

Merged
merged 1 commit into from
Feb 24, 2018

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Standardisation of website create function

Before

Website create function is highly specific & rather alarmingly includes deletion within create

After

The customised function is renamed to process & the places in the code identified to call it are changed to call the 'process' function.

Technical Details

To prevent hard-breakage a 'create' function still exists and if the calling code appears to be calling the 'old' create then process will be called. This is determined by the passed params as the old function took an array of website arrays & had 2 optional params. the api WILL pass an $ids array so we check if param 2 is empty or an array while deciding.

Comments

We've seen a string of fixes & regressions like these #11683 #11634 #11428 and I think we need to
a) get a sensible BAO / api working
b) determine if we agree that website type is optional & need not be unique - there could be flow on impacts on searches , reports & joins if they are not unique
c) handle what needs to be done with form input in a form function & remove the BAO process as & when we can

@yashodha @agh1 you have both been involved in previous patches around attempts to fix #11683 & may have opinions

@eileenmcnaughton
Copy link
Contributor Author

@colemanw a way to phase out create functions that also delete (can use the same approach on UFJoin)

@colemanw
Copy link
Member

For point B, is there any precedent already for enforcing them to be unique?

@eileenmcnaughton
Copy link
Contributor Author

@colemanw well the UI has enforced it (one way or another) for a long time - & people keep logging issues to change that. I don't know that the api has enforced it though

@eileenmcnaughton
Copy link
Contributor Author

@colemanw note that this PR is separate to us resolving b - it should be merged either way as it solves the 'WTF would you write a create function with a delete it in' issue

* @return bool|CRM_Core_BAO_Website
*/
public static function create($params, $contactID = NULL, $skipDelete = NULL) {
if ($skipDelete || ($contactID && !is_array($contactID))) {
Copy link
Member

Choose a reason for hiding this comment

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

What about being more specific instead of checking $skipDelete for truthyness check if it is not null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

on the off possibility someone was passing in FALSE in the past? I guess I can - although I changed the only places in core & suspect that it's not being called from outside of core

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK - done

@colemanw
Copy link
Member

Ok, merging. This is an internal function with api test coverage, so if the tests pass, it works.

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.

4 participants