NEW Control components written on GridFieldEditableColumns::handleSave #422
Conversation
…:handleSave Many objects contain a link back to the parent object (by nature of has_many), the same object that is being saved and triggering the GridFieldComponent to save its relational components in turn. This is often unnecessary and creates a 2N scenario for each record in the list, affecting performance. Especially when other onBefore/After/During write hooks are hanging off that class (e.g. fulltext search updates).
Internally the variables are private, so name isn't a big issue there. Internally to DataObject the method calls are `writeComponents` (called from `write`) and `skipWriteComponents()`, which is why the get/set functions in the EditableColumns component were named that way. However it is a bit confusing, and the variables that are passed can be either boolean, or a configuration array (which isn't very well documented by core)... adding to the confusion. By calling it a "write config" it is a bit clearer the purpose of the function calls and what the author of consuming code intends to happen at that point. This came as feedback on the [original PR][1] [1]: symbiote#350
|
Can you please create an issue, and in the issue include:
As is it's not clear to me what this does. |
|
Lets talk it through then. It contains a description and proceeds to describe a theoretical situation with examples of both expected and actual behaviour. This is the same branch with the same target as the referred to PR. The last update there instructed me to open a new PR, so I have. It might be easeir to reopen the old PR which contains all the previous conversation and context, instead of repeating it here. |
There description also doesn't describe how to set this scenario up, or how to check if it's happening (or not happening after this PR). It does show a "execution flow" but I don't understand what that flow is intending to show. The naming of the new API (including the word "config") suggests there's something going on beyond just a "do write" or "don't write" - what is the "config" that is being set? |
|
In addition to that, tracking this PR without an associated issue is difficult with the CMS Squad's current workflow. I requested an issue specifically be created because that helps track the PR, as well as providing a clear template in which to fill in the required information. An issue is required with the PR template, which you must have removed when creating this PR in favour of "Retry #350" which was the only context in the PR until I added the description from the previous one. Neither I nor Steve (the two people in the CMS Squad and therefore the most likely people to review and merge this) have much time to spare reviewing community PRs, especially right now with CMS 6 looming. The more information provided (preferably in the format the templates provide) the easier it is to justify spending time reviewing a PR and therefore the more likely it is to get merged. |
Retry #350
Many objects contain a link back to the parent object (by nature of has_many), the same object that is being saved and triggering the GridFieldComponent to save its relational components in turn. This is often unnecessary and creates a 2N scenario for each record in the list, affecting performance. Especially when other onBefore/After/During write hooks are hanging off that class (e.g. fulltext search updates).
E.g.
Action: Save Page
bad execution flow:
Write Thing1
=>Write Page
Write Thing2
=>Write Page
Write Page
good execution flow:
Write Thing1
Write Thing2
Write Page
In order to have the opportunity to allow components to perform the second, this PR adds API.