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

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

Merged
merged 1 commit into from
Jul 23, 2018

Conversation

monishdeb
Copy link
Member

Overview

There is regression found in Event's PCP as:
image
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
Copy link

civibot bot commented Jul 22, 2018

(Standard links)

@KarinG
Copy link
Contributor

KarinG commented Jul 22, 2018

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

@seamuslee001
Copy link
Contributor

@civicrm-builder re test this please

@eileenmcnaughton
Copy link
Contributor

eileenmcnaughton commented Jul 22, 2018

@KarinG Can you confirm if this fixes the issue - if you can we will ensure it is in 5.4.0

@petednz
Copy link

petednz commented Jul 23, 2018

@jitendrapurohit can you confirm if this might affect the client we are just starting with pcp fz-15712 and hence test etc

@KarinG
Copy link
Contributor

KarinG commented Jul 23, 2018

@Eileen - yes I’ll test - when we get back home. Earlier you wrote you will issue a 5.3.2?

@KarinG
Copy link
Contributor

KarinG commented Jul 23, 2018

So this PR produces another Fatal Error [just tested in our repo w/ the production site]. Will dig in the logs to find the details.

Reverting our repo back to how I fixed it originally:
$page->add('textarea', "pcp_personal_note", ts('Personal Note'), array('maxlength' => '250', 'style' => 'height: 3em; width: 40em;')); \

Hold on - I'm getting an unrelated Fatal Error - checking now.

Yep False alarm - this PR is works - phew!

Confirming edits:

[karin@louise Form]$ diff Registration.php.dist Registration.php
199,200c199,202
<     $this->_action = CRM_Utils_Request::retrieve('action', 'String', $this, FALSE);
< 
---
>     $this->_action = CRM_Utils_Request::retrieve('action', 'Alphanumeric', $this, FALSE, CRM_Core_Action::ADD); 
>     

and confirming that I've removed my override for pcp_personal_note in /CRM/PCP/BAO/PCP.php

@eileenmcnaughton
Copy link
Contributor

Merging based on @KarinG testing.

Re 5.3.2 - I changed the comment because I wasn't sure if we would issue or wait to do 5.4 since it turns out this regressed in 5.2 rather than 5.3 it could go either way.

@eileenmcnaughton eileenmcnaughton merged commit 9424244 into civicrm:5.4 Jul 23, 2018
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.

6 participants