| [09:39:25] | <mstenta[m]> | symbioquine: Wanted to follow up on the quick form entity validation topic a bit... digging into it now for the Movement and Group Membership quick forms. |
| [09:40:07] | <mstenta[m]> | cc paul121 for context: a Farmier user discovered that the Movement and Group Membership quick forms don't currently prevent circular group/location assignment. This means that it's possible to move and asset to itself, or assign a group as a member of itself. |
| [09:41:00] | <mstenta[m]> | Outside of the quick forms, we prevent this with validation constraints. |
| [09:42:15] | <mstenta[m]> | https://github.com/farmOS/farmOS/blob/3.x/modules/core/location/src/Plug... |
| [09:42:21] | <mstenta[m]> | https://github.com/farmOS/farmOS/blob/3.x/modules/asset/group/src/Plugin... |
| [09:42:48] | <mstenta[m]> | These are only checked when entity validation is run, however. |
| [09:43:33] | <mstenta[m]> | Drupal doesn't run validation when you run `$entity->save()` alone. You must first run `$violations = $entity->validate()` and deal with any violations that are returned. |
| [09:44:07] | <mstenta[m]> | In the case of a form, this should happen in formValidate(), before formSubmit(), so that they can be raised to the user and submission can be prevented. |
| [09:44:31] | <mstenta[m]> | We are not doing that in our quick forms currently. |
| [09:45:36] | <mstenta[m]> | The challenge is: in our quick forms, we don't create the entities until `formSubmit()`. So we can't just add `$entity->validate()` in there, because it's past the validation phase. |
| [09:46:03] | <mstenta[m]> | So it may require some refactoring to build the entities in formValidate() instead. |
| [09:48:08] | <mstenta[m]> | This has implications for our official quick form development documentation too... we should try to make it clear how to do this. |
| [09:51:19] | <mstenta[m]> | The thing is... in a lot of cases, running `$entity->validate()` isn't really necessary, by virtue of the fact that the quick form itself is only allowing valid values to be entered. This is the case for most "simple" quick forms. |
| [09:52:18] | <mstenta[m]> | In our quick form development documentation, for example, the form is so simple that it isn't really possible to create an invalid entity. |
| [09:52:19] | <mstenta[m]> | https://farmos.org/development/module/quick/ |
| [09:53:15] | <mstenta[m]> | The Movement and Group Membership quick forms are sort of outliers in this way. |
| [09:53:39] | <mstenta[m]> | But... we can't predict what validation constraints are applied by modules, so better to be safe I suppose. |
| [09:53:59] | <symbioquine[m]> | Most (all?) quick forms could benefit from backend/pre-save validation so I'd argue that it's a best practice for all quick forms to still call/handle $entity-validate()... |
| [09:54:16] | <mstenta[m]> | (eg: a constraint might say something like "Only the number 10 can be saved to a harvest log quantity!" - in which case the example in the docs wouldn't catch that) |
| [09:54:30] | <symbioquine[m]> | e.g. in the egg case, you could have race conditions where two people try and save harvests that cumulatively don't make sense given the flock size. |
| [09:55:15] | <symbioquine[m]> | By having the form written to include the validation step, it's just a matter of building a better constraint check to avoid that case. (DB-saving race conditions aside) |
| [09:55:21] | <mstenta[m]> | The other important layer here is: we provide some helper methods (via traits) to make it easier to generate entities in quick forms. But those SAVE the entities at the same time. |
| [09:55:43] | <mstenta[m]> | That's really the blocker here, from a technical standpoint. |
| [09:56:42] | <mstenta[m]> | In the example docs:... (full message at <https://matrix.org/oftc/media/v1/media/download/AaxxqSfeDg3qbM-JISbVanqr...) |
| [09:56:56] | <mstenta[m]> | This takes the array of values and creates the log entity and saves it. |
| [09:57:00] | <symbioquine[m]> | Similarly, if we imagine a transplanting case. Specific farms might want to add their own constraints around what makes sense. e.g. Don't allow multiple transplants of certain crop families (like lettuce) once they go into a bed location. |
| [09:57:19] | <mstenta[m]> | It does not call `$entity->validate()` - nor can it... because it's happening in `formSubmit()`. |
| [09:57:38] | <mstenta[m]> | So fixing this requires refactoring our quick form APIs, in other words. |
| [09:57:44] | <symbioquine[m]> | By always including the validation step in the quick form, that becomes a reliable extension mechanism so those conventions can be enforced simply by adding the constraints. |
| [09:58:07] | <symbioquine[m]> | All this is just IMHO of course :) |
| [09:58:16] | <mstenta[m]> | Agree on all points symbioquine. |
| [09:58:41] | <mstenta[m]> | The only problem is this is going to be a breaking change and will require significant changes to our quick form APIS. |
| [09:58:48] | <mstenta[m]> | s/APIS/APIs/ |
| [09:59:05] | <symbioquine[m]> | So a good change to go in 4.x? |
| [09:59:19] | <mstenta[m]> | It's necessary, though, and reveals the problems with the "simplification" we offer currently. |
| [09:59:40] | <mstenta[m]> | Yes, it would have to be 4.x. And it would mean anyone who wrote quick forms would need to refactor them for v4. |
| [10:00:24] | <mstenta[m]> | We should review in more detail, maybe on the next dev call, to think through the best ways forward. |
| [10:00:52] | <mstenta[m]> | For now, I'm going to have to figure out a workaround in 3.x for the Movement and Group Membership quick forms. |
| [10:01:21] | <symbioquine[m]> | Maybe it would be good to move this chat log into a forum topic so folks can review and respond async? |
| [10:01:21] | <mstenta[m]> | Either that or just accept that they have the potential to create invalid entities, and wait for v4... that may end up being easier. |
| [10:01:26] | <symbioquine[m]> | Then we can go through it next Thurs |
| [10:01:33] | <mstenta[m]> | I've only seen it happen once, and it's not too hard to fix after the fact (just manually edit the invalid log). |
| [10:03:34] | <mstenta[m]> | We could also take a more drastic measure in 3.x... we could add `$entity->validate()` in our helper functions, and throw an exception if violations are found. This would prevent the bad entities from being saved, but it would be poor UX. |
| [10:03:49] | <symbioquine[m]> | What if that createLog method were changed to call validate and throw and exception? It's not as good as handling the validation output properly, but might be better than allowing bad data to get persisted... |
| [10:03:57] | <mstenta[m]> | Might still be worth considering that, though... 🤔 |
| [10:04:00] | <symbioquine[m]> | You beat me to it |
| [10:04:06] | <mstenta[m]> | Ha |
| [10:04:48] | <mstenta[m]> | I can draft a quick PR that does that, to make the discussion easier |
| [10:04:57] | <mstenta[m]> | Pretty easy change |
| [10:05:08] | <symbioquine[m]> | The worst case there would be something like a "reverse parent-child relationship" quick form which then would have to be rewritten without the helpers. |
| [10:05:54] | <symbioquine[m]> | * The worst (probably imaginary) case there |
| [10:06:30] | <symbioquine[m]> | * The worst (probably imaginary) case there, * quick form (i.e. a form that takes to terms/assets and swaps which, * which one is the parent of the other) which then would |
| [10:06:48] | <mstenta[m]> | It's possible that this would break existing quick forms... but only in cases where they are saving invalid entities to begin with. So I think that can still be considered a "bug fix" and not a "breaking change"? |
| [10:07:29] | <symbioquine[m]> | I tend to agree, but curious if Paul knows or can think of other scenarios/considerations... |
| [10:08:22] | <mstenta[m]> | I'm finishing up work on "Gracefully handle invalid entity IDs in Views access checks" - so I'll take a quick stab at quick form entity validation next |
| [10:14:33] | <mstenta[m]> | https://github.com/farmOS/farmOS/pull/952 |
| [10:32:17] | * farmBOT has joined #farmos |
| [11:01:32] | <mstenta[m]> | OK here's a PR for quick form entity validation: https://github.com/farmOS/farmOS/pull/953 |
| [11:01:39] | <mstenta[m]> | (very quickly drafted - so please review!) |
| [11:01:51] | <mstenta[m]> | I need to head outside now :-) |
| [11:57:27] | <paul121[m]> | <symbioquine[m]> "What if that createLog method..." <- Throwing an exception came to mind as well... but agree it wouldn't be ideal. I'm not sure if there's really a way we can make this "easy" |
| [11:59:01] | <mstenta[m]> | Yea sort of a "last resort" to prevent invalid data. Ultimately quick forms should be responsible for validation. We just don't provide a great way of doing that with our "quick create methods" right now. |
| [11:59:24] | <mstenta[m]> | But in practice it's probably mostly more complex quick forms (like Movement/Group) that are most affected |
| [12:02:01] | <paul121[m]> | <symbioquine[m]> "I tend to agree, but curious..." <- Not sure about other scenarios or considerations. I think it's just something "you have to do" and there isn't really an easy way around it. Maybe one thing to consider is what if a quick form relies on existing bad data, and is facilitating an update/edit to an existing entity, and has validation issues irrelevant to its logic? Kinda a "what if" scenario. But I wonder what the message |
| [12:02:01] | <paul121[m]> | should be to the user.... would it ever make sense to let them override? 😅 |
| [12:02:11] | <paul121[m]> | I think I did implement validation checks with importing data from John Deere. If there is a validation issue it just skips those things and displays a message. Nothing too complex |
| [12:07:46] | <paul121[m]> | A more complex example though is the `farm_template` module. It handles entity validation and prevents saving if there are any issues. If the template is being used in a quick form, the validation prevents the quick form from... (full message at <https://matrix.org/oftc/media/v1/media/download/Afv6myKc01GTOjpmZFTy5h68...) |
| [12:14:18] | <paul121[m]> | I remember this all being a little tricky to figure out... the challenge is that you basically need to execute all of your logic twice: once for validation, once for submission. BUT during valdiation there is a trick where you... (full message at <https://matrix.org/oftc/media/v1/media/download/ASQJajzSKDpNAM0ASg6sKBtJ...) |
| [12:18:48] | <paul121[m]> | So.... for our quick forms maybe there is a strategy where we encourage ppl to put the "business logic" of the quick form into a separate method (not validate, not submit). This method is called on formValidate(), resulting entities are returned and saved to $form_state, and then submitForm() goes through and saves each of the entities? I think this would work for creating and updating entities... probably not for deleting entities |
| [12:18:49] | <paul121[m]> | nor other more complex logic... |
| [14:26:10] | <paul121[m]> | I recently met someone who is working on this "Software Gardening Almanack": https://software-gardening.github.io/almanack/introduction.html |
| [14:26:10] | <paul121[m]> | > Welcome to the Software Gardening Almanack, an open-source handbook of applied guidance and tools for sustainable software development and maintenance. |
| [14:26:10] | <paul121[m]> | I ran the almanack report and uploaded here. Some interesting metrics! https://gist.github.com/paul121/8f9e709bfab28edaa269d1b875733347 |
| [14:28:11] | <paul121[m]> | Seems we could consider adding a CITATION.cff file to help others know how to cite farmOS.. might be relevant for anyone that uses it for research/managing research data? |
| [16:20:12] | <mstenta[m]> | Neat! |
| [16:20:27] | <mstenta[m]> | 1.9 commits per day! Not bad 😄 |
| [16:21:01] | <mstenta[m]> | Over the course of ~12 years! |