Generate button links in PHP instead of smarty for ContributionView form

[mattwire]

Overview

ContributionView form has some impressive and probably insecure code to create the "link" buttons such as edit/delete. Even better the code is duplicated for top and botton!

Before

Fairly complex smarty code to generate button links for Edit/Delete on contributionview form.

After

Smarty logic recreated in PHP form (no attempt to clean up at this stage - it's meant to be a straight refactor). Smarty logic removed from top/bottom buttons and now using standard "linkButtons".

Technical Details

Comments

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

[eileenmcnaughton]

I agree this is a good idea - ie moving it to the php layer is just better.

Regarding how this intersects with the financialacls work - I think it makes sense to consolidate fixing that in this PR - so I have removed from the other.

We are removing handling of the smarty variables $canEdit and $canDelete so the assignment of them should be removed too.

UPDATE - my original thoughts about a hook were wrong - we can check if the contact has permission to update (canEdit) or delete (canDelete) using the api.

so the core code should check something like this when determining whether to show the update

$results = \Civi\Api4\Contribution::checkAccess()
  ->setAction('update')
  ->addValue('id', 202)
  ->execute();
foreach ($results as $result) {
  // do something
}

We'd need to check but it looks like the hook code is already in the extension to interact with that

civicrm-core/ext/financialacls/financialacls.php

Line 288 in 00d50ac

function _financialacls_civi_api4_authorizeContribution(\Civi\Api4\Event\AuthorizeRecordEvent $e) {

[eileenmcnaughton]

This section should wind up removable with the view bounce being done here and then all the other checks can be done with the checkAccess being used further down

[eileenmcnaughton]

this will simplify down to that api checkAccess

[eileenmcnaughton]

just yay

[mattwire]

@eileenmcnaughton What do you think about merging this first and then making the financialacl changes? In theory this is a "no change" PR that should just map the logic that was in smarty to PHP. I'm getting a bit muddled when looking through the changes needed as you describe above although I can see we'll end up with much cleaner code.

[eileenmcnaughton]

@mattwire as is this actually does break financial acls - see

But I'll put something up to make it clearer to you what I mean - I think it will be fairly clean

[mattwire]

as is this actually does break financial acls

Ah ok!

[eileenmcnaughton]

I think I lost my comment - this is the commit that should fix it - just trying to confirm - f6957ad

Still a couple more undefined vars (eg. below) - I'm not looking into those

[eileenmcnaughton]

Just confirming that I did some testing & f6957ad does make the financial acl part work - although if #22684 is not merged first then that commit causes a regression ( unconfirmed but logically I think so)

So it's just the undefined variables I think (& stopping the php layer assigns since no longer used)

[eileenmcnaughton]

@mattwire I decided to take another look at those variables & consolidated this PR & my PR into #22698

[eileenmcnaughton]

@mattwire we should probably close this as #22698 replaces + has fixes for the issues in this

[mattwire]

Closing in favour of #22698