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

Fix PHP8 warnings in Participant.tpl #26166

Conversation

larssandergreen
Copy link
Contributor

@larssandergreen larssandergreen commented May 7, 2023

Before

Lots of warnings on: standalone New Event Registration, Actions - Register participants for event when selecting contacts, New Event Registration for Contact (both live credit card and regular), Delete Event Registration and Edit Event Registration.

Send Notification was not being hidden as intended due to the div having id notify, which something else was targeting, at least for me.

After

No more warnings.

Send Notification hidden and shown as intended.

Technical Details

Some of this maybe isn't the prettiest, but I think better to check the context in the template rather than, for example, dealing with the three different variables for contact id that may or may not be present across three different classes.

@civibot
Copy link

civibot bot commented May 7, 2023

(Standard links)

@civibot civibot bot added the master label May 7, 2023
@larssandergreen larssandergreen force-pushed the Fix-PHP8-warnings-on-participant.tpl branch from a3c156c to b20313c Compare May 7, 2023 15:53
@larssandergreen larssandergreen changed the title Fix PHP8 warning in Participant.tpl Fix PHP8 warnings in Participant.tpl May 7, 2023
@@ -1740,8 +1740,6 @@ public function buildEventFeeForm($form) {
}
$form->assign('onlinePendingContributionId', $form->get('onlinePendingContributionId'));

$form->assign('paid', $form->_isPaidEvent);
Copy link
Contributor

Choose a reason for hiding this comment

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

This removal causes a problem since now it never shows the fee selections!

Copy link
Contributor

Choose a reason for hiding this comment

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

We seem to be back to the fees not appearing again. I see this is moved up to the top, but $form->_isPaidEvent isn't set until after that.

@larssandergreen larssandergreen force-pushed the Fix-PHP8-warnings-on-participant.tpl branch from b20313c to 20759d3 Compare May 17, 2023 21:50
@larssandergreen
Copy link
Contributor Author

Thanks @demeritcowboy, I've pushed a new version addressing those issues.

@demeritcowboy
Copy link
Contributor

I think all the warnings are now good but something isn't getting saved and the resulting participant record shows 0 fees paid and if you go edit the record it appears as though no fees were selected.

This seems tricky to get all the side-effects. Classic civi.

@larssandergreen larssandergreen force-pushed the Fix-PHP8-warnings-on-participant.tpl branch 2 times, most recently from 5db72eb to da2e68f Compare May 20, 2023 22:19
@larssandergreen
Copy link
Contributor Author

larssandergreen commented May 20, 2023

Thanks for the thorough r-run @demeritcowboy.

Of course, $paid couldn't be set earlier as I had it, because in most cases the event wasn't selected until after the page was loaded, so it wasn't being set true correctly as needed. Looking more closely at this template where $paid was used, it was only to conditionally display some top of the page instructions about how to record payments that could only possibly be shown if the user used Event Links - Register Participant, while if they selected the event on the form, that text would not be shown. So since it was rarely shown and not particularly useful anyways, I think we can just cut it in the name of simplicity and consistency and eliminate the need for $paid in this template (it is still used in EventFees.tpl and I've made sure it will always exist there).

@larssandergreen
Copy link
Contributor Author

Split into two PRs, per @eileenmcnaughton. Second PR is #26319

@demeritcowboy
Copy link
Contributor

jenkins retest this please

@larssandergreen larssandergreen force-pushed the Fix-PHP8-warnings-on-participant.tpl branch from bedbeaf to 9f512e1 Compare May 25, 2023 15:55
@demeritcowboy
Copy link
Contributor

Think this is good now.
I was consistently seeing something weird on the test site here if you arrived at the form from search contacts, but I can't reproduce it elsewhere so I'm not sure if it's some edge case specific to certain contacts or just a glitch. I do notice there is a long pause here between when you select the event and the fees appear, so am leaning toward glitch. And I can't see how it would be happening from this PR. It would show the words Event Level, but no fees, and then also have the Change Selections button as if it was an edit instead of add. 🤷

@demeritcowboy demeritcowboy merged commit cf6c1ff into civicrm:master May 25, 2023
@larssandergreen
Copy link
Contributor Author

Thanks @demeritcowboy for bearing with me on this one.
I am also seeing some occasional weirdness. Sometimes the event fees don't seem to load at all, sometimes weird things are shown, I think similar to what you describe with Change Selections but can't recall the exact details. I've seen it on this branch and on master and other branches, so I don't think it is anything to do with this PR. Can't figure out how to reproduce it.

In general, price sets seem to do weird things from time to time.

@eileenmcnaughton
Copy link
Contributor

Big picture I would love to separate the event fees loading form from the participant form - we have done that with others over time - e.g the core payment form. There is a gotcha around qfkeys though that I'm trying to recall

@larssandergreen larssandergreen deleted the Fix-PHP8-warnings-on-participant.tpl branch May 26, 2023 00:08
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.

4 participants