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

[REF] Make apiv3-specific helper generally available. #17169

Closed
wants to merge 1 commit into from

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Apr 25, 2020

Overview

Internal code cleanup - Make apiv3-specific helper generally available.

Before

No alternative to the discourages _civicrm_api_get_entity_name_from_camel

After

Recommended alternative CRM_Core_DAO::getEntityNameFromCamelName

Technical Details

This function should only be called from apiv3 but it's prevalence suggests it's more
generally useful. I have only done a couple of sample switches to use it. I think the
apiv4 one could in theory result in an error as is since there is no require_once for the
utils.php file

Comments

Once merged we ca switch across other calls & more noisily deprecate the function

@civibot
Copy link

civibot bot commented Apr 25, 2020

(Standard links)

@colemanw
Copy link
Member

I'm not sure I agree with this as a "canon" function to include in CRM_Core_DAO.
This function converts CamelCase to snake_case, but snake_case is pretty much deprecated. APIv4 doesn't use it or accept it. APIv3 accepts it but then immediately converts to CamelCase which is considered the canonical form of an entity.

At the very least I think the function is named incorrectly, as the current name suggests it is converting to something standard, which it is not. But I'm not convinced it belongs in CRM_Core_DAO. If anywhere I'd probably stick it in CRM/Utils/DeprecatedUtils.php.

@eileenmcnaughton
Copy link
Contributor Author

@colemanw there are 27 calls to this function - a good portion of them are doing what the apiv4 call is doing - figuring out the likely alias for the id field (not such an apiv4 thing) & it was in order to re-use that that I thought to do this. Since this approach - https://github.com/civicrm/civicrm-core/pull/16983/files#diff-a2c7ae339db570d160a69bd288516d11R90 seems silly to me

Moving this to deprecated utils seems clearly no worth doing so I would close in preference to that.

@colemanw
Copy link
Member

@eileenmcnaughton could you pls clarify what you mean by "what the apiv4 call is doing"?
The only time I see this fn in use by api4 is here:

// For legacy reasons the permissions are keyed by lowercase entity name
// Note: Convert to camel & back in order to circumvent all the api3 naming oddities
$lcentity = _civicrm_api_get_entity_name_from_camel(\CRM_Utils_String::convertStringToCamel(self::getEntityName()));

And that's only because, as the comment says, it's to honor a legacy hook contract.

@eileenmcnaughton
Copy link
Contributor Author

@colemanw it's taking 'activity' or 'OptionGroup' and figuring out that the alias is 'activity_id' ....

@colemanw
Copy link
Member

@eileenmcnaughton once again I find myself in need of a function like this. This time to automatically generate a table alias for a join (converting e.g. EntityTag to entity_tag)

I think my only complaint about this pr is the placement of the function. Since CRM_Core_DAO is the parent class for all dao and bao classes, it doesn't strike me as a great place to stick random utility functions. Why don't we move it to CRM_Utils_String, as it can go right beneath CRM_Utils_String::convertStringToCamel().

@colemanw colemanw reopened this May 15, 2020
@eileenmcnaughton
Copy link
Contributor Author

@colemanw I'm OK with String - although it is a string-munging that is logically specific to our DAO strings...

@colemanw
Copy link
Member

colemanw commented May 15, 2020

Yea if you look at CRM_Utils_String::convertStringToCamel() it is similarly specific. Maybe that one's not in a great place either, but there it is.

@totten
Copy link
Member

totten commented May 15, 2020

More grist for the mill:

  • CRM_Core_DAO_AllCoreTables - Provides lots of helpers for mapping between different formulations of the table names.
  • xml/schema/** and CRM_*_DAO_* - Provides lots of metadata about each entity (fields(), getTableName()). There is currently a uniqueName that appears on some of the 'id' columns, although there isn't a good helper to grab it.

Disclaimer/data-point: I've only skimmed and haven't fully wrapped my head around the concept of this mapping. Maybe there's a more distinctive name. For me, the phrase "entity name" evokes the UpperCamel formulation, as it in appears civicrm_api[34]($entity=='FooBar'...) and hook_civicrm_entityTypes ($entityTypes['CRM_Core_DAO_FooBar']['name']=='FooBar'). The idea

@colemanw
Copy link
Member

@eileenmcnaughton I've made the change and rebased this.

*
* @return string
*/
public static function convertNameToLowerCase(string $entity): string {
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 it needs to have the word Entity in the name since it's a special sort of name being converted

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 kinda prefer to put it in AllCoreTables but I can live with here)

Copy link
Member

Choose a reason for hiding this comment

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

Ok I changed the name.

This function should only be called from apiv3 but it's prevalence suggests it's more
generally useful. I have only done a couple of sample switches to use it. I think the
apiv4 one could in theory result in an error as is since there is no require_once for the
utils.php file
)), '_');
if (!$entity) {
// @todo - this should not be called when empty.
return '';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ideally add deprecation noise if this is hit

@colemanw
Copy link
Member

@eileenmcnaughton I've taken your suggestion to move it to AllCoreTables and done some additional cleanup: #17330

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