Saving Model with first-level BTM associations will fail silently, if the BTM field is not accessible

[smarek]

This is a (multiple allowed):

• bug

• enhancement

• feature-discussion (RFC)

• CakePHP Version: 3.7.3

• Platform and Target: linux php7.3 nginx

While dev/testing i've baked three models

• documents
• document_types
• document_types_documents

Having done some changes, i've manually added belongsToMany from Documents to DocumentTypes

Setting data correctly through Form->control('document_types._ids', ['options'=>$arr])

What was not working is saving data, and there is no warning/error about it.

DocumentsTable $_accessible was missing property 'document_types'=>true which corresponds with belongsToMany alias, adding it fixed everything.

So I'd like to know, if it is possible to make such situation developer-detectable, that Documents->patchEntity($document, $this->request->getData()) does not set the BTM fields (or generate appropriate entitites), because of the BTM alias/field accessibility

It is currently not even detectable from contents of $document (in this case) entity after patching, because neither dirty, original or other fields, that could hint such problem, do not reflect this.

Thank you

[markstory]

How would you want to find out about fields not being marshalled?

[smarek]

Well, as soon as possible, i guess /s

Either if it would be possible to handle non-accessible fields strictly (ie. trying to update such field would trigger full blown error), so I could turn that on for whole application and prevent such situations by getting rid of warnings/errors

Or at least some notice, when using Model->patchEntity($entity, $requestData), possibly using non-default settings, that would produce warnings (by default turned off), or hinting in the entity fields, after patching proceeds, that some dirty fields/assocs will be ignored on save.

Both of these could probably be turned on by debug mode of the application, and turned on/off on single-framework-api-call basis

[markstory]

Either if it would be possible to handle non-accessible fields strictly (ie. trying to update such field would trigger full blown error), so I could turn that on for whole application and prevent such situations by getting rid of warnings/errors

Wouldn't this make API endpoints more challenging to operate? Any unexpected field would cause an exception which doesn't create very robust applications.

Opt-in errors and warnings could work. If you remember to enable the opt-in warnings couldn't you also check the marshalled entity as well?

[smarek]

Yes, mentioned is not robust approach, however as I said, either only in debug mode, or debug mode and having said strict validation enabled.

Well, there is a difference imho between having the framework report errors (single point of validation in ORM implementation) and having to validate every entity patching on field-level/assoc-level (a lot of validation points and an insance amount of work on the developer side)

[markstory]

Enabling a strict mode on marshalling could work. I don't think that would require debug as well though.

$articles->patchEntity($article, $data, ['strict' => true]);

Could raise an error when any fields are part of the data that could not be marshalled into the entity graph.

[dereuromark]

I use a similar strict mode (default true here, however) for DTO patching. Makes sense to do that for entities, too.
The only question is if this should raise exceptions or just set error() for those fields/keys.

[smarek]

@markstory as long as I could set it as default for whole application or model, without need to modify code for every patchEntity call, i'm happy.
Also I vote for keeping default strict disabled, as that would break too many existing code-bases

@dereuromark error() for said fields/keys, which will prevent successful save into database thus propagate to business logic

[dereuromark]

Yeah, don't get me wrong: Of course it has to be disabled - for this specific use case - by default :) BC reasons but also because often you do not care here too much for too much input posted (can have other reasons like referer keeping).

[markstory]

@smarek You could use inheritance to replace patchEntity() and newEntity() and set strict mode on all calls. The ORM doesn't really have much in the way of global configuration right now and I'm not sure this warrants adding it.

[dereuromark]

See how Shim plugin did it for strict saving/deleting.