Add index_onCopy method to ListController behavior#943
Add index_onCopy method to ListController behavior#943marvindurot wants to merge 10 commits intowintercms:developfrom
Conversation
|
Oooo, I like this. Any chance you could hook this up to a list in the Backend, or perhaps the Test plugin so we can see it in action? Some review comments to follow. |
Thank you for your review! I will hook this up to a list in the Test plugin 😉 |
Co-authored-by: Ben Thomson <git@alfreido.com>
Co-authored-by: Ben Thomson <git@alfreido.com>
|
I made an example on Test plugin : wintercms/wn-test-plugin#17 😉 |
| if (array_key_exists('protected', $config) && $config['protected'] === true) { | ||
| $protectedColumns[] = $column; | ||
| } | ||
| } |
There was a problem hiding this comment.
I would prefer if the configuration was available under $listConfig->list and supported either an explicit allow list or an explicit ignore list (example below):
config_list.yaml
replication:
enabled: true
# one of the following, if both present then the most restrictive combination should be used
allowed: [name, type, status]
ignored: [id, slug, password]The allowed functionality can probably be implemented by doing $protected = array_diff($allowed, array_keys($record->getAttributes())
And if replication is enabled and neither is provided then we should default to looking at the model's fillable / guarded definitions (allowing explicit configuration in the config_list.yaml to overrule those values if desired).
@bennothommo any thoughts?
There was a problem hiding this comment.
@bennothommo further idea, should model replication be controlled solely through properties on the model itself, so we can then more easily reuse this logic for a "Duplicate" button in the form context for duplicating records one by one?
There was a problem hiding this comment.
I think we try and stick close to what Laravel has done, for feature parity. Laravel's replicate() method accepts fields as an "exclude" list, ie. exclude these fields from being replicated - I guess the assumption is that you don't want to have to have yet another array of fields you have to add new fields to. I also think that tying it to the guarded / fillable arrays might dilute what replicate is supposed to do - it's supposed to replicate an entire model after all.
There was a problem hiding this comment.
To answer your question about keeping the control within the model - I think that would be OK, but I would: a) allow the list controller to override this if a list of fields is provided, and b) stick with Laravel and make it an exclude list.
There was a problem hiding this comment.
We should also have an event fired on the model on replication, if there isn't one already.
There was a problem hiding this comment.
What concern do you have about also supporting allow lists rather than just ignore lists? Laravel's replicate() as implemented is a low level feature where developers have to explicitly call it and know what they're doing, this feature in Winter is meant for inclusion in backend lists (potentially all of them by default) and use by end users
There was a problem hiding this comment.
@bennothommo also what is your proposed implementation for the event? I'm fine leaving it out for now if we don't have a specific design for it yet.
There was a problem hiding this comment.
I don't see a purpose for an allow list. This is a copy function - in most cases, it will copy everything. If we add an allow list, that's an extra bit of functionality on top of Laravel that we need to track in the future for breakages on major updates.
There was a problem hiding this comment.
In conclusion what I am supposed to do in this pull request ?
There was a problem hiding this comment.
@marvindurot if you're still interested in having this merged then just ping me and I'll reopen. If you could submit a PR to the docs that should be all that's needed at this point.
marvindurot
force-pushed
the
feat/add-copy-method-to-list-behavior
branch
from
aef3368 to
66ae831
Compare
| 'delete_selected' => 'Supprimer la sélection', | ||
| 'delete_selected_empty' => 'Il n\'y a aucun enregistrement à supprimer', | ||
| 'delete_selected_confirm' => 'Confirmer la suppression des enregistrements sélectionnés ?', | ||
| 'delete_selected_confirm' => 'Confirmer la suppression des eanregistrements sélectionnés ?', |
|
This pull request will be closed and archived in 3 days, as there has been no activity in this pull request for the last 6 months. |
github-actions
bot
added
the
stale
bennothommo
removed
the
stale
|
@marvindurot are you still interested in this? |
|
This pull request will be closed and archived in 3 days, as there has been no activity in this pull request for the last 6 months. |
github-actions
bot
added
the
stale
3 participants
This PR adds new method
index_onCopymethod toListControllerbehavior.This method can be used to mass copy checked list elements like mass deletion works.
I also added a list column configuration variable to define which column is
protectedfrom mass copy.For more documentation about replicating models, see :