Implementing option to enforce the limit

[spekulatius]

Hello @patricknelson

I have implemented an option to enforce limits in the CMS by removing the "add" and "link existing" options. Are you interested to merge this in?

Cheers,
Peter

[patricknelson]

For safety, probably best to set this to >=.

[patricknelson]

First of all: Thank you for your PR. The first one for this project 😎👍

I could be misinterpreting this but: Currently the functionality is such that this component is already limiting, by default -- as long as this component is added to your GridField's configuration. The code you wrote does NOT prevent this from limiting. All I'm seeing is code that will only remove visual UI components (which may vary from implementation to implementation, by the way) in case that limit is reached.

So here's my list of things I'd suggest:

• I'm seeing that you added a method called ->enforceLimit() which is confusing, because the limit is already enforced and that fact doesn't seem to change if you call this method. I think instead this should be a setter (first of all) which uses different terminology. That is, it should instead be based on removing components, not if we enforce or not. This suggests that we actually have a totally different approach, ->setRemovedComponents([ ... array of component type names, each a string ... ]). This to me would be more open to reuse. That being said, initial version of this shouldn't make any assumptions at all and instead require you to specify which components to remove first. Maybe we can think of a sane default, but I'd need to research that first. That or maybe two methods, this one and ->addRemovedComponent(string). Thoughts?
• I like the idea of actually making it possible to make the actual enforcement of limited lists optional (i.e. that is the truncation of lists once you pass a certain count). If we do that, then we could create a totally different API called ->setEnforceLimits(bool), and of course, this would be enabled by default.

Let me know your thoughts and if I'm mistaken on any assumptions here. Thanks again.

[patricknelson]

Also, ->onAfterManipulate() is a method exposed for external use. If you call this here, it will override someone's ability to use it. Instead, please move this logic to ->getManipulatedData() and ensure the new updated logic doesn't conflict with the current $total > $this->maxItems check. In fact, your logic should go ABOVE the place where $this->onAfterManipulate is invoked, here https://github.com/patricknelson/silverstripe-gridfieldlimititems/blob/master/code/GridFieldLimitItems.php#L175 and likely below the if block above it.

[patricknelson]

Also, moving that logic which calls the callback setter reinforces my point about using a ->setRemovedComponents(...) setter above above which would need to be called on GridFieldLimitItems for anyone interested in utilizing this component removal feature once the limit is reached. I'd be interested in maybe also exposing those removed components as a configuration option (maybe null by default) in the constructor of the GridFieldConfig_LimitedRelationEditor helper.

[patricknelson]

Also, if you have time (pretty please) maybe be the first to add unit tests to this? Would only want coverage for what you write. If not, that's ok for now anyway (I've been slacking). See #1

[spekulatius]

Hello Patrick,

thanks for coming back to me so quick :) Currently I'm quite busy because I'm going on leave soon. So this might need to wait a bit more - I'll address most of this as soon as I find time.

Peter

[patricknelson]

No worries, there's no rush.

[patricknelson]

Ping @spekulatius -- any updates here?