dev/core#463 Add in ability for Extensions to use their Extension Utils class to s… #12986

seamuslee001 · 2018-10-23T00:11:29Z

…upply ts Functions in DAOs

Overview

This allows for Civix or similar to ensure that when the DAO is generated that it uses instead of ts E::ts and that civix can insert the relevant use statement to use the Extensions' utils class

Before

No extension utils in DAO

After

Extension utils exists

Technical details

When creating a new entity (e.g DiscountItem) via an extension the new entity will need a DAO class. Our Civix code currently supports this - at least somewhat - but will not wrap any translatables with ts rather than the (correct for extensions) E::ts . Adding this to core will allow civix to leverage it to better create extension DAO objects

civibot · 2018-10-23T00:11:30Z

(Standard links)

If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

eileenmcnaughton · 2018-10-23T02:49:19Z

I'm +1 on this but I want to give @mlutfy & @totten a chance to comment if they have any thoughts

seamuslee001 · 2018-10-23T06:32:54Z

Note that there is a corresponding PR for civix here it totten/civix#143

mlutfy · 2018-10-23T12:38:34Z

In terms of use-case, makes sense to me!

colemanw · 2018-10-23T15:21:13Z

xml/templates/dao.tpl

@@ -8,6 +8,11 @@
 * (GenCodeChecksum:{$genCodeChecksum})
 */

+
+{if $extension}
+extesniontuils


eileenmcnaughton · 2018-10-23T20:45:08Z

I've added merge-ready. Ideally @totten will comment & we should hold a bit for that - but not that long as we know Tim can't look at everything & this is fairly minor

mlutfy · 2018-10-23T21:09:11Z

xml/templates/dao.tpl

+
+{if $extension}
+extensionutils
+{/if}


What does this do?

@mlutfy it outputs a string that then CiviX swaps out for a use CRM_<extesnionname>_Utils as E; here https://github.com/totten/civix/pull/143/files#diff-b961fdd9b92320d1fd7efa5fd0f49cfeR92 I was struggling to get gencode to respect the spaces between use and the CRM... so did it this way around

totten · 2018-10-23T21:20:21Z

xml/templates/dao.tpl

@@ -90,7 +95,7 @@ class {$table.className} extends CRM_Core_DAO {ldelim}
                 'name'      => '{$field.name}',
                                                                      'type'      => {$field.crmType},
 {if $field.title}
-                                                                      'title'     => ts('{$field.title}'),
+                                                                      'title'     => {if $extension}E::ts{else}ts{/if}('{$field.title}'),


On a functional level, 👍 for having any DAO's in the extension use the extension's translation domain.

FWIW, when writing the template for the extension utils class, I explicitly wanted to avoid making CRM_Foobar_ExtensionUtil classes into a public contract (or inter-module contract). For example, it does not extend any shared base class or shared interface. The original/intended audience was only the author of the civix-generated module -- it was not designed to be shared/public-contract.

Why?

(1) There are already public contracts for things like ts(...'domain' => ...) -- using the E class an inter-module contract would be duplicative and (2) civix is only supposed to be a template/skeleton generator. If an author wants to totally muck with all the code in there, then that should still be workable. But if others depend on civix-isms, then the dependency graph can get weird.

This patch is interesting because it lets the downstream inject the statement like use CRM_Foobar_ExtensionUtil as E -- technically, the core code doesn't know anything about CRM_Foobar_ExtensionUtil, but it does know about this virtual symbol E and E::ts().

Would this patch mean that core code has some dependency on a civix convention? Honestly, it's right on the line. I think it's technically, currently on the safe side of the line. My hesitation is more with the precedent -- if other things in core start to depend on having an E class, then E will become a public contract leading eventually to funny loops.

Just to put it out there, here are a couple alternatives:

(a) Have the civix generator pass the extension-key org.example.foobar or translation-domain for use in dao.tpl. In dao.tpl, it conditionally passes this for ts(... ['domain' => ...]).

(b) Have the civix generator pass an expression for the translation callback (e.g. $ts = 'CRM_Foobar_Extension::ts'). In dao.tpl, it uses this instead of a straight call to ts().

@totten yeh i see your point but also I think this also about ensuring we have a separation between Core (ts) and the Extensions routing through their E utils class

OK, so a routing point is useful because it gives you some leverage -- you make a change in one spot, and it applies to several use-cases.

E::ts() doesn't give us that:

Within an extension, E::ts() is used by newer PHP code, but it's not used by Smarty code, Javascript code, or older PHP code. To make E::ts() useful as a router, one would have to rework all those other call-paths.

If you do make a change in E::ts(), it's liable to get trounced later on -- because it's part of the autogenerated *.civix.php boilerplate.

The common-denominator is that all paths lead to the global ts() function (with a domain argument). If one wants the benefit of routing (i.e. a leverage point where you can structurally manipulate all translations of a given extension/domain/purpose), then you currently have to look at the global ts().

@totten so then would you suggest that its better to perhaps add an extra function called like ts to extension DAOs that then calls the global ts function with the domain param?

seamuslee001 · 2018-10-29T22:26:49Z

@totten i believe this is now updated as per our discussion just now

seamuslee001 · 2018-10-29T23:06:13Z

@totten @eileenmcnaughton i have confirmed that this produces the same output for Core as previous and confirmed that it works for extensions. This also does not bind Core to Extension purposes as previous and resolves Tim's concerns

eileenmcnaughton · 2018-10-29T23:16:54Z

@seamuslee001 looks good to me

seamuslee001 · 2018-10-29T23:44:26Z

Jenkins re test this please

…ls class to supply ts Functions in DAOs Update DAO generation using ts function name as per conversation with Tim

seamuslee001 · 2018-10-30T04:32:04Z

test this please

seamuslee001 · 2018-10-30T05:46:54Z

Jenkins reset this please

seamuslee001 · 2018-10-30T05:58:55Z

merging as per the tag

seamuslee001 requested a review from totten October 23, 2018 00:11

civibot bot added the master label Oct 23, 2018

seamuslee001 mentioned this pull request Oct 23, 2018

Ensure that Extension Ts function is used in place of ts in DAOs and output… totten/civix#143

Merged

eileenmcnaughton added the sig:extension support Supports being able to integrate extensions with CiviCRM label Oct 23, 2018

colemanw reviewed Oct 23, 2018

View reviewed changes

xml/templates/dao.tpl Outdated

@@ -8,6 +8,11 @@

* (GenCodeChecksum:{$genCodeChecksum})

*/

{if $extension}

extesniontuils

Copy link

Member

colemanw Oct 23, 2018

Choose a reason for hiding this comment

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

Typo?

seamuslee001 force-pushed the lab_core_463 branch from f06d55b to 136e5e3 Compare October 23, 2018 20:41

seamuslee001 changed the title ~~Add in ability for Extensions to use their Extension Utils class to s…~~ dev/core#463 Add in ability for Extensions to use their Extension Utils class to s… Oct 23, 2018

eileenmcnaughton added merge on pass merge ready PR will be merged after a few days if there are no objections and removed merge on pass labels Oct 23, 2018

mlutfy reviewed Oct 23, 2018

View reviewed changes

totten reviewed Oct 23, 2018

View reviewed changes

seamuslee001 force-pushed the lab_core_463 branch from 136e5e3 to 68b687f Compare October 29, 2018 22:26

eileenmcnaughton added merge on pass and removed merge ready PR will be merged after a few days if there are no objections labels Oct 29, 2018

dev/core#463 Add in ability for Extensions to use their Extension Uti…

8af97d9

…ls class to supply ts Functions in DAOs Update DAO generation using ts function name as per conversation with Tim

seamuslee001 force-pushed the lab_core_463 branch from 68b687f to 8af97d9 Compare October 30, 2018 02:06

seamuslee001 merged commit 81e3283 into civicrm:master Oct 30, 2018

seamuslee001 deleted the lab_core_463 branch October 30, 2018 05:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dev/core#463 Add in ability for Extensions to use their Extension Utils class to s… #12986

dev/core#463 Add in ability for Extensions to use their Extension Utils class to s… #12986

seamuslee001 commented Oct 23, 2018 •

edited by eileenmcnaughton

Loading

civibot bot commented Oct 23, 2018

eileenmcnaughton commented Oct 23, 2018

seamuslee001 commented Oct 23, 2018

mlutfy commented Oct 23, 2018

colemanw Oct 23, 2018

eileenmcnaughton commented Oct 23, 2018

mlutfy Oct 23, 2018

seamuslee001 Oct 23, 2018

totten Oct 23, 2018 •

edited

Loading

seamuslee001 Oct 23, 2018

totten Oct 23, 2018 •

edited

Loading

seamuslee001 Oct 23, 2018

seamuslee001 commented Oct 29, 2018

seamuslee001 commented Oct 29, 2018

eileenmcnaughton commented Oct 29, 2018

seamuslee001 commented Oct 29, 2018

seamuslee001 commented Oct 30, 2018

seamuslee001 commented Oct 30, 2018

seamuslee001 commented Oct 30, 2018

dev/core#463 Add in ability for Extensions to use their Extension Utils class to s… #12986

dev/core#463 Add in ability for Extensions to use their Extension Utils class to s… #12986

Conversation

seamuslee001 commented Oct 23, 2018 • edited by eileenmcnaughton Loading

Overview

Before

After

Technical details

civibot bot commented Oct 23, 2018

eileenmcnaughton commented Oct 23, 2018

seamuslee001 commented Oct 23, 2018

mlutfy commented Oct 23, 2018

colemanw Oct 23, 2018

Choose a reason for hiding this comment

eileenmcnaughton commented Oct 23, 2018

mlutfy Oct 23, 2018

Choose a reason for hiding this comment

seamuslee001 Oct 23, 2018

Choose a reason for hiding this comment

totten Oct 23, 2018 • edited Loading

Choose a reason for hiding this comment

seamuslee001 Oct 23, 2018

Choose a reason for hiding this comment

totten Oct 23, 2018 • edited Loading

Choose a reason for hiding this comment

seamuslee001 Oct 23, 2018

Choose a reason for hiding this comment

seamuslee001 commented Oct 29, 2018

seamuslee001 commented Oct 29, 2018

eileenmcnaughton commented Oct 29, 2018

seamuslee001 commented Oct 29, 2018

seamuslee001 commented Oct 30, 2018

seamuslee001 commented Oct 30, 2018

seamuslee001 commented Oct 30, 2018

seamuslee001 commented Oct 23, 2018 •

edited by eileenmcnaughton

Loading

totten Oct 23, 2018 •

edited

Loading

totten Oct 23, 2018 •

edited

Loading