[2011-04-12] FieldSet::insertBefore() sometimes doesn't insert at all

[silverstripe]

[2011-04-12] FieldSet::insertBefore() sometimes doesn't insert at all

[silverstripe-issues]

comment by: @sminnee (sminnee)
created at: 2012-04-18

---

I'd say it should throw an InvalidArgumentException. Easily caught.

[jonom]

I just came on to make nearly the same proposal but found this one. I think if the $insertBefore argument is invalid, the field should be inserted at the start rather than the end of the FieldList.

[tractorcow]

In the alternative, an exception should be thrown to indicate that the "existing field" could not be found.

I think so.

I just came on to make nearly the same proposal but found this one. I think if the $insertBefore argument is invalid, the field should be inserted at the start rather than the end of the FieldList.

This runs the risk of issues that would otherwise be emitted to the user to be silently hidden, and never fixed.

[jonom]

I think the proposal to always add the field is more practical. I expect this method to act just like 'insert' but with the added convenience of being able to specify a preferred position. Developers don't always know for a sure that the field they're referencing will exist, but as OP said, they will usually want to add the field anyway. A module author might expect a core field to be available for instance, but it might have been removed by user code. Or a field may be available on most pages, but hidden on a few page types. Throwing an error would break the app and might be quite tricky to fix, whereas inserting a field in an unexpected position isn't likely to have a big impact. Forcing developers to check for the existence of a field before calling insertBefore seems like overhead that could be avoided.

Maybe we could meet half way and add a third argument like $strict, to determine what should happen if the field can't be found. If $strict=false add the field to the fieldlist anyway. If $strict=true throw an exception. Default to false? :)

[dhensby]

I agree with @jonom that it should just work and that an exception is a bit much for adding fields to the CMS.

[sminnee]

The current implementation of this is that insertBefore() returns false when it fails to insert, and that it also traverses CompositeFields when attempting to insert.

So, I think the fix for this is to add a $appendIfMissing = true flag to the method signature, and set that to false for the CompositeField traversla.

[robbieaverill]

Fixed with #8427 in the SS 4.3.x release line