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

ENH: Add performance fixes on saving editable columns #341

Merged
merged 1 commit into from
Jun 16, 2022

Conversation

satrun77
Copy link
Contributor

@satrun77 satrun77 commented Jun 1, 2022

The changes in the PR to address the issue described in silverstripe/silverstripe-userforms#773

This might not eliminate the issue completely, but it is an improvement to the current code.

Changes include,

  • Use one query to fetch all items needed to be saved, instead of multiple queries.
  • Only save items that are changed.

@satrun77 satrun77 force-pushed the pull/performance-fixes branch from 33b16f1 to 8b2a541 Compare June 1, 2022 02:10
@satrun77 satrun77 marked this pull request as ready for review June 2, 2022 00:38
Copy link
Collaborator

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

If we change the protected method to private then there's no API change so we can do a patch release meaning we release it much faster.

Please rebase/retarget to the 3.4 branch

Also prefix your commit message and the title of the PR with ENH as this is an enhancement

src/GridFieldEditableColumns.php Outdated Show resolved Hide resolved
src/GridFieldEditableColumns.php Show resolved Hide resolved
src/GridFieldEditableColumns.php Outdated Show resolved Hide resolved
src/GridFieldEditableColumns.php Outdated Show resolved Hide resolved
@satrun77 satrun77 force-pushed the pull/performance-fixes branch from 8b2a541 to 3334a66 Compare June 15, 2022 04:51
@satrun77 satrun77 changed the base branch from 3 to 3.4 June 15, 2022 04:51
@satrun77 satrun77 changed the title Add performance fixes on saving editable columns ENH: Add performance fixes on saving editable columns Jun 15, 2022
@satrun77 satrun77 requested a review from emteknetnz June 15, 2022 04:53
Copy link
Collaborator

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

Have tested locally - I noticed a syntax error - I fixed on my local and this seemed to work correctly.

Happy to merge after this is fixed


if (!$item || !$item->canEdit()) {
// Skip not found item, or don't have any changed fields, or current user can't edit
if (!$item || !$this->isChanged($item, $fields) || !$item-canEdit()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (!$item || !$this->isChanged($item, $fields) || !$item-canEdit()) {
if (!$item || !$this->isChanged($item, $fields) || !$item->canEdit()) {

- Use one query to fetch all items needed to be saved.
- Only save items that are changed.
@satrun77 satrun77 force-pushed the pull/performance-fixes branch from 3334a66 to 547ec8a Compare June 16, 2022 01:02
@satrun77 satrun77 requested a review from emteknetnz June 16, 2022 01:02
@emteknetnz emteknetnz merged commit 0405d91 into symbiote:3.4 Jun 16, 2022
@emteknetnz
Copy link
Collaborator

I've released 3.4.1 for you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants