Refactor CRM_Utils_Api::getEntityName($this) in From addSelect method

[mallezie]

Yesterday i noticed something strange when trying to extend the EventInfo form in a civicrm extension.

I'm not sure if i'm doing something wrong, or how this should be improved, so i'm posting it here first for some discussion.

I tried to create an adjusted form for the eventinfo form. (In my use case to create specific event type add forms). So i created an extension in the namespace CRM_CustomEvent. There i started by creating a specific manageEvent class
class CRM_CustomEvent_Page_ManageCursus extends CRM_Event_Page_ManageEvent
to extend the Manage event (civicrm/event/manage) page. This works as expected. So i tried to do the same for the EventInfo form.
class CRM_CustomEvent_Form_ManageEvent_CursusInfo extends CRM_Event_Form_ManageEvent_EventInfo
This however fails. The reason is that this form has an addSelect form element, which automatically tries to detect the used entity.
(In the add Select declaration)
  function addSelect($name, $props = array(), $required = FALSE) {
    if (!isset($props['entity'])) {
      $props['entity'] = CRM_Utils_Api::getEntityName($this);
    }
...
This goes to the CRM_Utils_Api::getEntityName($this) function which tries to find the corresponding entity. This tries to use the parts of the class name, to find that entity. In the case of CRM_Event_Form_ManageEvent_EventInfo it finds it in the second part of the class name.
In my custom class however class CRM_CustomEvent_Form_ManageEvent_CursusInfo it doesn't and it crashes on the exception in  the CRM_Utils_Api::getEntityName($this) function.
The solution i use for now is to rename my class from class CRM_CustomEvent_Form_ManageEvent_CursusInfo to class CRM_Event_Form_ManageEvent_CursusInfo, however this sort of 'pollutes' the extension structure, and feels like a hack. So i actually want to remove this 'classnaming hack'.

If i don't call the form building method from the parent class, i can recreate it by giving explicitly the entity type to the addSelect methods, but i actually just want to extend the form, and adjust some of the elements, instead of completely recreating it.

Possible options, could be:

• Explicitly add the entity type in addSelect methods where the class name isn't the entity type, adn remove the other pattern matchings in static function getEntityName($classNameOrObject)
• Add the entity type as a class attribute, so we can explicitly set it when overriding this class
• ... Other suggestions welcome ;-) [/tt]
  

I'm not sure which option would be the best way to go. I'm willing to file an issue and PR, if i get some direction to know wich way is best.

[Coleman Watts]

So to clarify what you are doing: you're overriding the form class, and then overriding the buildQuickForm function, and somewhere inside your function you are calling parent::buildQuickForm(); Is that right? Would be helpful for us visual learners if you could paste in your code if that's not right.

[mallezie]

That's indeed what i'm doing ;-)


class CRM_Event_Form_SearchCursus extends CRM_Event_Form_SearchEvent {

  /**
   * Build the form
   *
   * @access public
   *
   * @return void
   */
  public function buildQuickForm() {
    parent::buildQuickForm();

    $event_type = CRM_Core_OptionGroup::values('event_type', FALSE);

    foreach ($event_type as $eventId => $eventName) {
      // Remove bivak checkbox
      if($eventName == 'Bivak'){
        $this->removeElement("event_type_id[$eventId]");
      }
    }
  }
}


I'm adjusting the build form (in this case i remove one of the event types).  And the only way i'm able to do this is when i name this class CRM_Event_Form_SearchCursus, that's sort of the base of my 'problem'.

[Coleman Watts]

Ok 2 things:

• I agree that addSelect could be better. In fact it could be a lot better because the very name is limiting. It should be called addField and the widget type can be fetched from the schema. As for cleaning up the hackish getEntityName function, I agree we should nip that in the bud. It was only added recently so fairly easy to uproot. A better solution would be for every class that extends CRM_Core_Form to declare a public property $entityType.
• To address your specific implementation. I think that what you are doing does not require overriding the CRM_Event_Form_SearchEvent class at all. I think you would be better served by implementing hook_civicrm_buildForm

[mallezie]

For point 2, my example isn't correct actually, since it should be about subclassing an Form which adds a select2 in its buildQuickForm Method. This could indeed done differently, with hook_civicrm_buildForm, but i wan't to keep the original form as well, and only access this on a other URL.

But to adress point 1, perhaps we could introduce this one bit at a time, and start by removing one special case of the CRM_Utils_Api::getEntityName($this); at a time, and add the property where we removed the special case.

[totten]

For point 2, my example isn't correct actually, since it should be about subclassing an Form which adds a select2 in its buildQuickForm Method. This could indeed done differently, with hook_civicrm_buildForm, but i wan't to keep the original form as well, and only access this on a other URL.

Good point.

If you can excuse some ranting... Extending a form is an interesting approach which isn't used very often (or, at least, isn't discussed very often). I'm not sure why... guess: hooks are documented/publicized, so folks may assume that they're less risky/more stable/more maintainable. There's a kernel of truth to this - but it doesn't really apply to the architecture in Civi 1.x-4.x and hook_buildForm. IMHO, extending a core class is just as valid as using hook_buildForm. In both cases, the fundamental maintainability risks are driven by (high) coupling to the target form class.

But to adress point 1, perhaps we could introduce this one bit at a time, and start by removing one special case of the CRM_Utils_Api::getEntityName($this); at a time, and add the property where we removed the special case.

Incremental improvement are basically always welcome.

In this case, I think you've stepped into the middle of a long-running conversation about a set of architectural changes that are due/ripe. Several folks (including Coleman and me) have been "chomping at the bit" to make a series of changes like this; the overhaul is a big undertaking, and we try to exercise some discipline [F1], but when an opportunity (such as this thread) arises to advocate for it, it's too tempting. One must say something. From this perspective, Coleman's suggestion is actually a pretty conservative/restrained/incremental take on the project.

[F1] "Discipline": It would be tempting to abandon all work and all communication and all obligation and focus on overhauling the form layer -- preferably while sitting with a laptop on a big cushy chair by a beach in Hawaii. Alas, reality beckons, and one heeds. Discipline.

[mallezie]

Thanks for the reply.

The reasons for my approach are maintainability especially, i want to override the form on 3 different ways, depending on the url, and that makes it more difficult to do with the hooks. The hooks are especially great for making 'smaller' customizations to the form, that's why i'm going for the overriding approach.

I'm interested in the long running overhaul conversations, but as you state, since sitting in Hawaii, isn't exactly an option (sounds great though), the overhaul should best happen in smaller iterations. (Could be great to get consensus over an end-result however, so we can iterate towards it.). Especially for this change, the goal is clear. (Remove CRM_Utils_Api::getEntityName($this)). (And perhaps a further goal to overhaul addSelect even more). I'll try to drive this change further home with incremental changes.

[mallezie]

For completion, also Drupal is going with a sort of incremental approach to remove all hooks. They started the overriding approach more and more in D8 (although forms didn't make it entirely), but that's probably the plan for D9.
I can see this change in this context to go for a more OO-driven approach.

[Coleman Watts]

To explain what Tim is alluding to, what we would really like to do is ditch QuickForm and Smarty completely, and build a shiny new flexible form system that does cool stuff like drag-n-drop fields, allowing you to override core forms and tweak them however you wish.
So whenever I make an improvement to CRM_Core_Form like the new addSelect and entityRef methods, it feels like a slightly hollow victory.
Holing ourselves up (a beach sounds nice but I wouldn't want to get sand in my keyboard) for a couple months to develop it as a skunkworks project would be awesome, but might not leave enough people home at the core team to keep the lights on.