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

Migrate JobLog page to API4 #24606

Merged
merged 1 commit into from
Sep 26, 2022
Merged

Conversation

braders
Copy link
Contributor

@braders braders commented Sep 24, 2022

Overview

Migrate JobLog page to API4

Before

The JobLog page was written in the traditional CiviCRM way using the DAO object. This led to notices on the JobLog screen from undefined array keys:

Screenshot 2022-09-24 at 11 58 36

Given that up to 1000 rows are shown at the time this could lead to significant noise in the site logs.

After

The JobLog page now uses API4. API4 keys are always set (unlike DAO objects which didn't set properties for NULL values), meaning that no undefined array key warnings occur.

To faciltate this JobLog entitites are now exposed over the API.

I have also done some minor tidy-up of the JobLog template:

  • The $sj variable was being set, but not referenced. Now removed.
  • jobName and jobRunUrl are now always set, reducing reliance on empty in the Smarty template (which I think is preferable when in CIVICRM_SMARTY_DEFAULT_ESCAPE mode).

Comments

I'm never quite sure how quickly PRs make it into a release, so the @since 5.56 is a bit of a guess on the new API entity.

I settled on @searchable secondary, but I think it's borderline whether this should be secondary or none. I don't really mind either way.

See also dev/core#2486

@civibot
Copy link

civibot bot commented Sep 24, 2022

(Standard links)

@civibot civibot bot added the master label Sep 24, 2022
@braders braders force-pushed the joblog-api4 branch 2 times, most recently from 9dfdbec to 5665c3b Compare September 24, 2022 11:17
@eileenmcnaughton
Copy link
Contributor

I gave this an r-run & it worked. I also dug into the permissions a little bit & will put up a follow up that tweaks core permissions & switches to checkPermissions false

@eileenmcnaughton
Copy link
Contributor

I put up this follow up #24619

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.

2 participants