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

[REF] Make use of recently added default pager size setting in Reports #20273

Merged
merged 1 commit into from
May 17, 2021

Conversation

seamuslee001
Copy link
Contributor

Overview

This makes use of the recently added default_pager_size setting in reports

Before

Constant used, no reference to setting

After

Setting used

ping @demeritcowboy @colemanw

@civibot
Copy link

civibot bot commented May 12, 2021

(Standard links)

@civibot civibot bot added the master label May 12, 2021
*
* @return array
*/
public function limit($rowCount = self::ROW_COUNT_LIMIT) {
public function limit($rowCount = NULL) {
if (CRM_Utils_System::isNull($rowCount)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I see isNull isn't deprecated yet but it does unexpected things sometimes. Was this copy/paste or is there a reason to use it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was wanting to get the function if $rowCount was NULL rather than say 0 had been passed into the function

Copy link
Contributor

Choose a reason for hiding this comment

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

In php 7.4 there's a nifty $rowCount ??= $this->getRowCount();. For php 7 in general could do $rowCount = $rowcount ?? $this->getRowCount(); or there's regular is_null().

/**
* Class constructor.
*/
public function __construct() {
parent::__construct();

$this->setRowCount(\Civi::settings()->get('default_pager_size') ?? 50);
Copy link
Contributor

Choose a reason for hiding this comment

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

It shouldn't be necessary to specify ?? 50 since settings() knows to take it from the file definition so it only needs to be in one place.

@seamuslee001 seamuslee001 force-pushed the report_default_pager branch from 1d3434a to 11630f6 Compare May 17, 2021 07:46
@seamuslee001
Copy link
Contributor Author

@demeritcowboy I believe I have updated this to what you wanted now

@demeritcowboy demeritcowboy merged commit 902737f into civicrm:master May 17, 2021
@seamuslee001 seamuslee001 deleted the report_default_pager branch May 18, 2021 00:39
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