dev/core#272 : Fatal Error (Regression) on PCP pages associated with Events

[monishdeb]

Overview

There is regression found in Event's PCP as:

Regression is caused by https://github.com/civicrm/civicrm-core/pull/12056/files#diff-20d61aa96c8fdcd879d2b760f13ccc74R456

Before

Fatal error on Event's page

After

Fixed the error.

Technical Details

Here addField('pcp_personel_note', ...) expects $form->_action which is not allotted to CRM/Event/Form/Registration/Register.php. This patch assigned a default action to its parent class like we do for all other Civi forms. Alternatively we can provide the 'action' parameter i.e.

- $page->addField('pcp_personal_note', array('entity' => 'ContributionSoft', 'context' => 'create', 'style' => 'height: 3em; width: 40em;'));
+ $page->addField('pcp_personal_note', array('entity' => 'ContributionSoft', 'context' => 'create', 'action' => 'create', 'style' => 'height: 3em; width: 40em;'));

But ideally we should always ensure that a form has action to render form elements for intended purpose.

Comments

ping @eileenmcnaughton @KarinG

[civibot]

(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).

[seamuslee001]

i feel like this should probably be changed to alphanumeric.

[monishdeb]

You mean:
$this->_action = CRM_Utils_Request::retrieve('action', 'String', $this, FALSE, CRM_Core_Action::ADD) ?

I did a grep on CRM codebase and all such occurrences I passing string value as default. Do

$ grep -irn --color "CRM_Utils_Request::retrieve('action'" CRM

[seamuslee001]

i mean $this->_action = CRM_Utils_Request::retrieve('action', 'Alphanumeric', $this, FALSE, 'add')

[monishdeb]

Oh ok. I think it would be best to have:

$this->_action = CRM_Utils_Request::retrieve('action', 'Alphanumeric', $this, FALSE, CRM_Core_Action::ADD);

[seamuslee001]

Yep happy with that 👍 just figure Alphanumeric is bit more strict than string

[KarinG]

Link to my GitLab reporting the issue: https://lab.civicrm.org/dev/core/issues/272

[KarinG]

Thanks @monishdeb and @seamuslee001 - I will try/test that in production today once it passes tests

[seamuslee001]

@civicrm-builder re test this please

[eileenmcnaughton]

5.4 version merged & there is a plan to do a 5.3.2 - merging