| [20:51:17] | * farmBOT has joined #farmos |
| [07:46:09] | <mstenta[m]> | Evan Kelley: Interesting! Just looking at the core issue you discovered in the SFI chat: https://www.drupal.org/project/drupal/issues/2395845 |
| [07:46:43] | <mstenta[m]> | Based on quick skim of that, if adding `$form['#attached']['library'][] = 'core/drupal.form';` to the forms fixes it, then do that! I don't see any problem with that approach. |
| [07:47:20] | <mstenta[m]> | Remember there is a `PodsFormBase.php` which all the PODS forms inherit from, so you can probably just add it there, and it will fix all of them. |
| [07:52:14] | <mstenta[m]> | Evan Kelley: I'm glad you found this - we may want to look into whether or not we should add this to farmOS Quick Forms in general. I'll have to test to see if they are susceptible to this bug as well, but based on reading it sounds like they might be. |
| [08:29:46] | <evered[m]> | 🌤️ |
| [08:30:20] | <mstenta[m]> | Morning evered ! |
| [10:46:49] | <mstenta[m]> | paul121 jgaehring I'm adding "GOAT Report Out" to the agenda for next week's monthly call, with a few bullet points for some of the discussions that we can highlight. Let me know if there are any that you'd like to add! I can update the agenda (or feel free to do it yourself). |
| [10:47:17] | <mstenta[m]> | Starting with: |
| [10:47:17] | <mstenta[m]> | - OFN |
| [10:47:18] | <mstenta[m]> | - Conventions |
| [10:47:18] | <mstenta[m]> | - ... (there are so many, I'm sure the list will grow) |
| [10:51:36] | <symbioquine[m]> | <mstenta[m]> "This comment is interesting..." <- I found this blog post which is doing something similar to what that comment suggests: https://www.previousnext.com.au/blog/how-create-and-expose-computed-prop...... (full message at <https://libera.ems.host/_matrix/media/r0/download/libera.chat/123ed26cfa...) |
| [10:57:08] | <mstenta[m]> | symbioquine: was just going to ping you :-) https://www.drupal.org/project/farm/issues/3314131#comment-14723473 |
| [10:57:29] | <symbioquine[m]> | Was just looking over that issue :) |
| [10:57:39] | <symbioquine[m]> | Hadn't seen the new comment though |
| [10:58:03] | <mstenta[m]> | Don't want to lose track of this one... |
| [10:58:39] | <mstenta[m]> | Ah that's interesting that it uses `onFieldDefinitionCreate()` from the `field_definition.listener` class... |
| [10:58:41] | <mstenta[m]> | I |
| [10:59:02] | <symbioquine[m]> | Yeah |
| [10:59:14] | <mstenta[m]> | I've had to do a few things with that service recently, so I wonder if this means that service is responsible for updating the field map |
| [11:00:04] | <mstenta[m]> | In theory (high level) there are probably two sides to solving this... |
| [11:00:05] | <symbioquine[m]> | There's also https://www.drupal.org/project/drupal/issues/3129179 (though it doesn't make any proposal about where that `rebuildBundleFieldMap()` should be getting called. |
| [11:00:12] | <symbioquine[m]> | * getting called.) |
| [11:01:01] | <mstenta[m]> | 1. Ensuring that OUR code (which adds `hook_farm_entity_bundle_field_info()`) rebuilds the field map when necessary (probably via `hook_modules_enabled()` so it runs whenever new modules get enabled) |
| [11:01:01] | <mstenta[m]> | 2. Add an update hook to FIX any missing fields on existing installs |
| [11:01:37] | <symbioquine[m]> | Makes sense |
| [11:02:13] | <mstenta[m]> | I'm guessing we can do (1) in a way that will gracefully relinquish control to Drupal core if/when the issue is resolved upstream |
| [11:02:22] | <mstenta[m]> | (check if the field is in the field map, and only add it if it's not) |
| [11:03:02] | <mstenta[m]> | This may not necessarily need to BLOCK the stable release... but I wanted to make sure we don't forget to consider it and ask that question |
| [11:03:31] | <mstenta[m]> | It's certainly a bug, but perhaps edgy enough that we can do it post-2.0.0 |
| [11:06:36] | <symbioquine[m]> | I could go either way. I'm blocked on it, but I can understand if the intersection between current write API clients and custom fields is small enough to justify punting a bit. |
| [11:07:19] | <mstenta[m]> | Yea - I just mean if everything else is ready to go on 2.0.0, maybe this doesn't need to be a blocker |
| [11:07:35] | <symbioquine[m]> | I've at least got to find a work-around for myself, but that's my problem :) |
| [11:08:18] | <mstenta[m]> | Yea! I'm curious if we can hack together a simple PHP script that would just rebuild the field map on-demand |
| [11:08:20] | <mstenta[m]> | As a proof of concept for the ultimate fix |
| [11:08:41] | <mstenta[m]> | Have you ever used `drush php-script`? It's a quick way to test stuff |
| [11:09:18] | <symbioquine[m]> | I can't remember for sure, but I vaguely recall a past work-around involving that. |
| [11:09:56] | <symbioquine[m]> | Might have been for the issue where I couldn't upgrade drush so I had to call the php version of some method directly. |
| [11:11:20] | <mstenta[m]> | I'm gonna try something real quick... |
| [11:12:18] | <symbioquine[m]> | mstenta[m]: Famous last words 😬 |
| [11:15:31] | <mstenta[m]> | symbioquine: So... we have two ways to add bundle fields... do you know if both of them are affected? Or just those added via this hook? |
| [11:15:50] | <mstenta[m]> | (The other way is via the PHP bundle class) |
| [11:16:01] | <mstenta[m]> | https://farmos.org/development/module/entities/#bundle-fields |
| [11:16:09] | <mstenta[m]> | vs https://farmos.org/development/module/fields/#bundle-fields |
| [11:16:47] | <symbioquine[m]> | Hmmm, you mean like the "nickname" field for animals? |
| [11:16:47] | <mstenta[m]> | (the former is for adding them to entity types defined by the same module, the latter is for adding them to entity types defined by ANOTHER module) |
| [11:16:50] | <mstenta[m]> | yea |
| [11:17:08] | <symbioquine[m]> | I don't think that's affected since I have code that sets that |
| [11:17:12] | <symbioquine[m]> | I can test real quick |
| [11:17:13] | <mstenta[m]> | ok great |
| [11:17:18] | <mstenta[m]> | ok yea if you can |
| [11:17:25] | <mstenta[m]> | that would be a much bigger issue if that didn't work |
| [11:17:36] | <symbioquine[m]> | Agreed |
| [11:21:13] | <symbioquine[m]> | This works;... (full message at <https://libera.ems.host/_matrix/media/r0/download/libera.chat/19313ee29d...) |
| [11:21:33] | <mstenta[m]> | Ok great |
| [11:21:34] | <symbioquine[m]> | https://github.com/farmOS/farmOS/blob/7694b2cbd3ee02884ddceba3f8e0bae5e6... |
| [11:22:19] | <mstenta[m]> | Looking through farmOS core's implementations of that hook now... I see three (apart from tests): |
| [11:22:20] | <mstenta[m]> | 1. `farm_equipment` uses it to add the `equipment` field to all log types (to specify which equipment asset performed a task) |
| [11:22:20] | <mstenta[m]> | 2. `farm_quick` uses it to add the `quick` field to all assets and logs (to specify which quick form created a record) |
| [11:22:20] | <mstenta[m]> | 3. `farm_sensor_listener` uses it to add a `public_key` field to `sensor` assets (for legacy v1 listeners) |
| [11:22:46] | <mstenta[m]> | Question: does this prevent READING (and filtering) on those fields via the API? or only writing? |
| [11:23:57] | <mstenta[m]> | (1) and (2) are important... especially being able to WRITE the `equipment` field (1) |
| [11:24:08] | <mstenta[m]> | If READING isn't an issue, then (2) doesn't matter too much... |
| [11:24:15] | <mstenta[m]> | And (3) is very minor |
| [11:25:03] | <mstenta[m]> | Re: (1) and (2) I'm questioning now why we are adding those as "bundle" fields and not "base" fields - because they are being added to ALL bundles anyway |
| [11:25:17] | <mstenta[m]> | So we may be able to easily transition those to "base" fields to fix them |
| [11:26:25] | <symbioquine[m]> | Yeah, reading and filtering works fine. It's just writing that's broken. |
| [11:26:26] | <mstenta[m]> | I'd like to ask paul121 if he has any memory of why those are defined as "bundle" fields and not "base" fields tho... |
| [11:26:38] | <mstenta[m]> | Ok good to know |
| [11:33:33] | <mstenta[m]> | ACTION sent a code block: https://libera.ems.host/_matrix/media/r0/download/libera.chat/9f248e0c20... |
| [11:34:03] | <mstenta[m]> | That's a start... it just loads all the bundle field definitions added via `hook_farm_entity_bundle_field_info()` |
| [11:34:20] | <mstenta[m]> | You can put that in `sites/all/scripts/rebuild-field-map.php` and then run it via `drush php-script sites/all/scripts/rebuild-field-map.php` |
| [11:34:40] | <mstenta[m]> | So I'd be curious if we can figure out how to manually update the field map... |
| [11:34:50] | <mstenta[m]> | Using this to test it |
| [11:36:08] | <symbioquine[m]> | Cool! |
| [11:36:44] | <mstenta[m]> | Maybe just:... (full message at <https://libera.ems.host/_matrix/media/r0/download/libera.chat/b07d1a59ef...) |
| [11:36:58] | <mstenta[m]> | Maybe you could test that? |
| [11:36:58] | <mstenta[m]> | (Might want to backup your `db` dir first) |
| [11:37:00] | <symbioquine[m]> | yeah |
| [11:37:12] | <mstenta[m]> | (So you can reset) |
| [11:37:41] | <symbioquine[m]> | I'll test that shortly here... |
| [11:38:19] | <mstenta[m]> | Cool! I'm going to do a little more research on the issue overall... curious to understand how this relates to core vs `entity` module |
| [11:38:35] | <mstenta[m]> | Because that will inform how we ultimately should fix it |
| [11:39:43] | <mstenta[m]> | It *sounds* like it's just a core issue related to `hook_entity_bundle_field_info()`... and NOT related to `entity` module's "bundle plugin" stuff (which is what allows us to define bundle fields in the bundle PHP classes) |
| [11:40:11] | <mstenta[m]> | The `hook_farm_entity_bundle_field_info()` hook that we added in farmOS core was added so that we could allow OTHER modules to add to those, basically... |
| [11:40:54] | <mstenta[m]> | And Drupal core's `hook_entity_bundle_field_info()` does not do that, IIRC... And there's also an open issue that suggested that hook is going to change, so we decided to make our own to provide a forward-thinking BC layer (again IIRC) |
| [11:41:19] | <mstenta[m]> | https://www.drupal.org/node/2346347 |
| [11:41:44] | <mstenta[m]> | Related tracking issue in our queue: https://www.drupal.org/project/farm/issues/3194206 |
| [11:42:26] | <mstenta[m]> | We are definitely pushing the envelope a little bit with these bundle fields... using `entity` module + our custom hook as additional layers on top of Drupal core's field system, which didn't fully support bundle fields the way we wanted |
| [11:42:40] | <mstenta[m]> | So I'm optimistic that we can fix this bug in our layer |
| [11:43:18] | <mstenta[m]> | If you can confirm that the script above fixes it, then I think the next steps might be pretty clear cut... |
| [11:44:20] | <mstenta[m]> | (Hmm although I wonder if that `onFieldDefinitionCreate()` is also going to try to CREATE the field...) |
| [11:44:28] | <mstenta[m]> | (We might need to dig into that method and isolate the field map bit only) |
| [11:44:33] | <symbioquine[m]> | Maybe a dumb question, but couldn't the `buildFieldDefinitions` method just invoke a hook that lets modules add bundle fields in the way that already works? |
| [11:44:49] | <mstenta[m]> | (Or dig deeper into the `entity` module to figure out why it's not being called for these fields) |
| [11:45:14] | <mstenta[m]> | What `buildFieldDefinitions` method? |
| [11:45:31] | <mstenta[m]> | Oh you mean in the bundle classes? |
| [11:46:12] | <mstenta[m]> | > (Or dig deeper into the `entity` module to figure out why it's not being called for these fields) |
| [11:46:12] | <mstenta[m]> | Yea! That might be what I mean by this ^ |
| [11:46:26] | <symbioquine[m]> | mstenta[m]: e.g. in https://github.com/farmOS/farmOS/blob/04bf771d91cde8276b67090d2b97c4e455... |
| [11:46:26] | <mstenta[m]> | The `entity` module is responsible for calling all of those `buildFieldDefinitions()` methods... |
| [11:47:06] | <mstenta[m]> | BUT we're doing some tricky stuff to add MORE definitions via our `hook_farm_entity_bundle_field_info()`... so it seems those are being missed |
| [11:47:14] | <symbioquine[m]> | symbioquine[m]: or maybe https://github.com/farmOS/farmOS/blob/04bf771d91cde8276b67090d2b97c4e455... |
| [11:47:43] | <mstenta[m]> | https://github.com/farmOS/farmOS/blob/2.x/modules/core/entity/src/Bundle... |
| [11:48:03] | <mstenta[m]> | ACTION sent a code block: https://libera.ems.host/_matrix/media/r0/download/libera.chat/281ac40432... |
| [11:48:50] | <mstenta[m]> | So to summarize: `entity` (contrib) module adds the ability to define bundle fields in those PHP classes via `buildFieldDefinitions()`... but our custom code is responsible for adding more fields to that via `hook_farm_entity_bundle_field_info()` |
| [11:49:02] | <mstenta[m]> | So I think this is our fault (or oversight) |
| [11:49:28] | <mstenta[m]> | Maybe we need something else in our `class FarmEntityBundlePluginHandler extends BundlePluginHandler` |
| [11:49:35] | <mstenta[m]> | To get them into the field map |
| [11:50:46] | <mstenta[m]> | Digging into `entity` module code now to refresh on how all that works... |
| [11:56:23] | <mstenta[m]> | Here is where `entity` calls: `onFieldDefinitionCreate()` |
| [11:56:26] | <mstenta[m]> | https://git.drupalcode.org/project/entity/-/blob/8.x-1.x/src/BundlePlugi... |
| [11:58:02] | <mstenta[m]> | Hmm... seems like this should be working... 🤔 |
| [11:58:48] | <mstenta[m]> | Oh wait maybe not! |
| [12:02:45] | <symbioquine[m]> | In that script, the `\Drupal::service('field_definition.listener')->onFieldDefinitionCreate($bundle_field);` line fails with; |
| [12:02:57] | <symbioquine[m]> | ACTION uploaded an image: (5KiB) < https://libera.ems.host/_matrix/media/r0/download/matrix.org/CkGnJsjvBZQ... > |
| [12:03:17] | <symbioquine[m]> | Haven't gotten to the bottom of that yet... |
| [12:04:04] | <mstenta[m]> | Oh hmm |
| [12:04:21] | <mstenta[m]> | Try adding this right above the `onFieldDefinitionCreate()` line: |
| [12:04:21] | <mstenta[m]> | `$bundle_field->setTargetEntityTypeId($entity_type);` |
| [12:04:34] | <mstenta[m]> | Might also need: |
| [12:04:42] | <symbioquine[m]> | Ah, yeah I was wondering if it might need something like that |
| [12:04:47] | <mstenta[m]> | `$bundle_field->setTargetBundle($bundle);` |
| [12:05:12] | <mstenta[m]> | Yea I'm guessing our hook doesn't add that kind of info to the definition, but it is needed (and gets added downstream by `entity` module somewhere) |
| [12:06:18] | <mstenta[m]> | I'm going to have to jump off to take over with my daughter in a bit... but currently setting up a test module so I can step through the existing code w/ debugger |
| [12:06:35] | <symbioquine[m]> | Cool, take care |
| [12:20:44] | <mstenta[m]> | Well, didn't make it super far, but it does seem that `onFieldDefinitionCreate()` in `entity` module is NOT running for the field in my test module |
| [12:20:53] | <mstenta[m]> | Next step is to figure out why... |
| [12:21:19] | <mstenta[m]> | I think you've successfully nerd-sniped me with this symbioquine 😅 |
| [12:21:25] | <symbioquine[m]> | Got it working... |
| [12:21:29] | <symbioquine[m]> | ACTION sent a php code block: https://libera.ems.host/_matrix/media/r0/download/libera.chat/30b22294c5... |
| [12:21:36] | <symbioquine[m]> | followed by a `drush cr` |
| [12:22:30] | <symbioquine[m]> | ACTION uploaded an image: (98KiB) < https://libera.ems.host/_matrix/media/r0/download/matrix.org/tqbIuJmMiED... > |
| [12:22:41] | <symbioquine[m]> | ^ Note the `quick` field is set |
| [12:23:19] | <symbioquine[m]> | > <@symbioquine:matrix.org> Able to reproduce it with the quick module enabled;... (full message at <https://libera.ems.host/_matrix/media/r0/download/libera.chat/5be0694e7b...) |
| [12:24:18] | <symbioquine[m]> | mstenta[m]: Could it be because the field definition doesn't have the name (or those other attributes) set? |
| [12:26:25] | <mstenta[m]> | Awesome!! |
| [12:26:44] | <mstenta[m]> | grokking... |
| [12:26:54] | <mstenta[m]> | so you also had to set the name? that makes sense too |
| [12:27:00] | <symbioquine[m]> | Yeah |
| [12:27:10] | <mstenta[m]> | > Could it be because the field definition doesn't have the name (or those other attributes) set? |
| [12:27:10] | <mstenta[m]> | Maybe |
| [12:27:24] | <symbioquine[m]> | and I took `\Drupal::keyValue('entity.definitions.bundle_field_map')->deleteAll();` from https://www.drupal.org/project/drupal/issues/3129179#comment-14688474 |
| [12:27:39] | <symbioquine[m]> | Which was (especially?) important because I'd run it previously without some of those attributes set. |
| [12:27:40] | <mstenta[m]> | Oh yea, so that just clears the field map? |
| [12:27:46] | <symbioquine[m]> | yep |
| [12:28:02] | <mstenta[m]> | Not sure I understand why that's necessary... 🤔 |
| [12:28:03] | <symbioquine[m]> | Even then, it didn't take effect until I cleared the cache also |
| [12:28:26] | <symbioquine[m]> | mstenta[m]: I suspect it wouldn't be if I was starting from scratch... |
| [12:28:34] | <mstenta[m]> | ok |
| [12:28:57] | <symbioquine[m]> | But I didn't like seeing |
| [12:28:58] | <symbioquine[m]> | ACTION uploaded an image: (45KiB) < https://libera.ems.host/_matrix/media/r0/download/matrix.org/ngDydsTDBKd... > |
| [12:29:12] | <symbioquine[m]> | ^ that entry with the empty string key :) |
| [12:29:16] | <mstenta[m]> | Well I'm going to focus a bit of time stepping through the `entity` (and `farm_entity`) module code to see if I can see where/why it isn't getting added... |
| [12:30:52] | <mstenta[m]> | Ahh hmm actually I think I know why... |
| [12:31:33] | <mstenta[m]> | `onFieldDefinitionCreate()` gets called by `BundlePluginInstaller::installBundles()`... but that only happens when NEW bundles are being installed |
| [12:31:34] | <mstenta[m]> | NOT when fields are being added to existing bundles |
| [12:31:43] | <mstenta[m]> | So yea... this all makes sense |
| [12:31:55] | <mstenta[m]> | And suggests we need to add some logic to `farm_entity` to cover this |
| [12:32:08] | <mstenta[m]> | (and ideally an automated test) |
| [12:32:36] | <symbioquine[m]> | So we might be back to the solution of using `hook_modules_enabled` and comparing whether the field is already in the field map? |
| [12:32:57] | <mstenta[m]> | Maybe... unless there's a class in `entity` that we can override/extend |
| [12:33:03] | <mstenta[m]> | Like we're doing with some of the others |
| [12:33:18] | <symbioquine[m]> | I see |
| [12:33:20] | <mstenta[m]> | That feels like the "right" solution |
| [12:33:31] | <symbioquine[m]> | Makes sense |
| [12:33:34] | <mstenta[m]> | But... it may be that `entity` doesn't handle this AT ALL |
| [12:33:38] | <mstenta[m]> | (updates to existing bundles) |
| [12:34:31] | <mstenta[m]> | I'm not seeing anything that suggests it does... |
| [12:34:33] | <mstenta[m]> | > So we might be back to the solution of using `hook_modules_enabled` and comparing whether the field is already in the field map? |
| [12:34:34] | <mstenta[m]> | So yea... this might be the only way |
| [12:35:08] | <mstenta[m]> | I'm curious if we can add a simple automated test for this to show that it fails currently |
| [12:35:32] | <mstenta[m]> | I haven't done any tests that use the API yet... but I think we have some... |
| [12:35:32] | <symbioquine[m]> | The corresponding removal case on module disable sounds harder |
| [12:35:41] | <mstenta[m]> | Mmmmm yea good point |
| [12:36:12] | <mstenta[m]> | Well... there is also `hook_modules_uninstalled()` I think |
| [12:36:39] | <mstenta[m]> | https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Extension... |
| [12:36:47] | <symbioquine[m]> | but we'd need a way to tell which of the fields came from `farm_entity_bundle_field_info` and now need to be removed right? |
| [12:37:05] | <mstenta[m]> | So maybe we just need a generic function that compares the field map against the list of bundle fields, and updates the map accordingly |
| [12:37:15] | <mstenta[m]> | Yea |
| [12:37:18] | <symbioquine[m]> | Maybe just clear it and rebuild? |
| [12:37:19] | <mstenta[m]> | Yea maybe that's easiest |
| [12:37:43] | <symbioquine[m]> | That script that starts with clearing seems to work - though I don't know if there would be any unintended side-effects |
| [12:38:00] | <symbioquine[m]> | Seems like the most obvious one is that two such hooks couldn't co-exist |
| [12:38:26] | <mstenta[m]> | What do you mean? |
| [12:38:41] | <mstenta[m]> | Oh two hooks that cleared/rebuilt? |
| [12:38:45] | <symbioquine[m]> | yeah |
| [12:38:49] | <mstenta[m]> | Gotcha |
| [12:39:09] | <symbioquine[m]> | If they rebuilt using different sources of information, whichever one ran second would win |
| [12:39:25] | <mstenta[m]> | True |
| [12:39:33] | <symbioquine[m]> | The base and normal bundle fields would survive, but the fields they added after that wouldn't be safe. |
| [12:39:53] | <mstenta[m]> | I think we are deep enough into farmOS-y edge cases here that that would be pretty unlikely :-) |
| [12:39:56] | <symbioquine[m]> | Might not be a big deal assuming there's only those two ways in farmOS for bundle fields to be added. |
| [12:40:17] | <mstenta[m]> | Yea |
| [13:04:15] | <symbioquine[m]> | So it turns out that `\Drupal::keyValue('entity.definitions.bundle_field_map')->deleteAll();` is dangerous and we shouldn't do that... |
| [13:04:43] | <symbioquine[m]> | It was wiping out fields like `plant_type` on the `seed` asset. |
| [13:04:44] | <mstenta[m]> | Okedoke! |
| [13:05:26] | <symbioquine[m]> | For future reference here's a one liner to get the field map for assets; `drush php-eval 'print_r(array_keys(\Drupal::service("entity_field.manager")->getFieldMap()["asset"]))'` |
| [13:06:02] | <symbioquine[m]> | I restored an older DB backup and then the following script worked... |
| [13:06:25] | <symbioquine[m]> | ACTION sent a php code block: https://libera.ems.host/_matrix/media/r0/download/libera.chat/682794fa1e... |
| [13:07:02] | <mstenta[m]> | Ok |
| [13:07:14] | <mstenta[m]> | So... I realized: I don't know how these bundle fields are being installed in the first place 🤔 |
| [13:07:39] | <mstenta[m]> | Trying to figure that out now... because obviously they are being INSTALLED (not only when the bundle itself is installed) |
| [13:08:00] | <mstenta[m]> | So maybe there is still an opportunity to jump in at that point |
| [13:08:26] | <symbioquine[m]> | mstenta[m]: Yeah, that seems especially clear now that I've found that they don't come back after `\Drupal::keyValue('entity.definitions.bundle_field_map')->deleteAll()` |
| [13:09:09] | <symbioquine[m]> | symbioquine[m]: whereas some of the fields do get automatically re-added to the field map somehow |
| [13:09:20] | <symbioquine[m]> | (might only be base fields) |
| [13:09:25] | <mstenta[m]> | So yea actually... I think this really does just boil down to the core issue you found originally... |
| [13:09:47] | <mstenta[m]> | I think the `entity` module is just calling core hooks to get the bundle fields to be installed... but delegating to core to install them |
| [13:10:00] | <mstenta[m]> | But core is not adding bundle fields to the field map |
| [13:10:11] | <symbioquine[m]> | Sounds right |
| [13:10:39] | <mstenta[m]> | So yea... maybe coming back to `hook_modules_installed()` :-) |
| [13:10:46] | <mstenta[m]> | (as a general solution to that) |
| [13:11:31] | <symbioquine[m]> | It doesn't seem to hurt anything (that I've run into yet) to call `\Drupal::service('field_definition.listener')->onFieldDefinitionCreate($bundle_field);` redundantly |
| [13:12:11] | <mstenta[m]> | ok that's good |
| [13:12:12] | <symbioquine[m]> | As a work-around, couldn't I just put that in my `hook_farm_entity_bundle_field_info`? |
| [13:12:41] | <symbioquine[m]> | I mean the script probably is also a viable work-around for me, but I'm just asking... |
| [13:13:59] | <mstenta[m]> | Hmm I don't think that would work... because `hook_farm_entity_bundle_field_info()` is just defining the fields... it isn't installing them yet... so if you run `onFieldDefinitionCreate()` in that hook then it may run before the field is actually installed |
| [13:14:15] | <mstenta[m]> | Maybe it would work, depending on how all the downstream code works, but feels risky |
| [13:14:15] | <symbioquine[m]> | oh |
| [13:14:51] | <mstenta[m]> | Oh and also: `hook_farm_entity_bundle_field_info()` is run for other things besides installing the fields... |
| [13:14:59] | <mstenta[m]> | So that would mean it would run that in places where fields aren't being installed |
| [13:18:55] | <mstenta[m]> | > The corresponding removal case on module disable sounds harder |
| [13:18:55] | <mstenta[m]> | This is still the bit that might be challenging I think... |
| [13:19:27] | <mstenta[m]> | `hook_modules_uninstalled()` runs after the module is installed, so we won't have access to the `hook_farm_entity_bundle_field_info` provided by it |
| [13:20:02] | <mstenta[m]> | but maybe we can figure out how to run that hook even after it has been uninstalled |
| [13:20:09] | <mstenta[m]> | and then just remove those fields from the field map |
| [13:21:15] | <symbioquine[m]> | Can we use the `provider` attribute of the field definition? https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Field%21B... |
| [13:21:35] | <mstenta[m]> | OH! |
| [13:21:46] | <mstenta[m]> | Good idea! |
| [13:21:47] | <symbioquine[m]> | Or maybe we just need to set that and core will take care of removing it on uninstallation? |
| [13:21:54] | <mstenta[m]> | Goooooood question! We DO set that! |
| [13:22:34] | <mstenta[m]> | https://github.com/farmOS/farmOS/blob/7694b2cbd3ee02884ddceba3f8e0bae5e6... |
| [13:22:50] | <mstenta[m]> | I remember paul121 added that |
| [13:22:59] | <symbioquine[m]> | Ah, I see it https://github.com/farmOS/farmOS/blob/c92d181c45b2158b360318edb277ff175f... |
| [13:23:04] | <mstenta[m]> | // Set the provider for each field the module provided. |
| [13:23:04] | <mstenta[m]> | // This is required so that field storage definitions are created in the |
| [13:23:04] | <mstenta[m]> | // database when the module is installed. |
| [13:23:05] | <mstenta[m]> | yea! |
| [13:23:10] | <mstenta[m]> | I wonder if it handles uninstall already |
| [13:23:23] | <symbioquine[m]> | But I'd need to set that in the script also... |
| [13:23:27] | <mstenta[m]> | Well let's give something a try... I'm sketching up the `hook_modules_installed()` right now... |
| [13:23:38] | <mstenta[m]> | So maybe we can test this without the script |
| [13:30:27] | <mstenta[m]> | ACTION sent a code block: https://libera.ems.host/_matrix/media/r0/download/libera.chat/64cb6a0ec9... |
| [13:30:49] | <mstenta[m]> | Completely untested... this should go at the top of `farm_entity.module` |
| [13:30:51] | <symbioquine[m]> | Any idea why `invokeAllWith` wouldn't be available when calling it from `drush php-script`? |
| [13:30:58] | <mstenta[m]> | Hmm not sure |
| [13:31:17] | <symbioquine[m]> | k, no worries |
| [13:31:46] | <mstenta[m]> | The above hook intends to update the field map for modules when they are installed... but will not update existing fields |
| [13:31:57] | <mstenta[m]> | So you may need to test by uninstalling and reinstalling your module (or make a test module) |
| [13:32:12] | <mstenta[m]> | I'm going to test with my test module to see if it works... |
| [13:32:27] | <symbioquine[m]> | Yeah, I'll probably uninstall/reinstall the farm_quick module to test |
| [13:34:11] | <symbioquine[m]> | > <@mstenta:matrix.org> ```... (full message at <https://libera.ems.host/_matrix/media/r0/download/libera.chat/18750dcf6a...) |
| [13:34:19] | <mstenta[m]> | oops broken... |
| [13:34:41] | <mstenta[m]> | Hmm not sure that's necessary |
| [13:34:43] | <mstenta[m]> | Because we're setting it elsewhere |
| [13:34:49] | <mstenta[m]> | But I guess we'll see |
| [13:34:57] | <symbioquine[m]> | Makes sense |
| [13:35:32] | <symbioquine[m]> | Depends where the bundle field uninstallation logic sources that value I guess. |
| [13:36:34] | <mstenta[m]> | * ```... (full message at <https://libera.ems.host/_matrix/media/r0/download/libera.chat/c77f20e276...) |
| [13:36:47] | <mstenta[m]> | I'll update my paste above as I test this... |
| [13:38:06] | <mstenta[m]> | i think it worked! |
| [13:39:50] | <symbioquine[m]> | I don't see that `hasImplementations` method in https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Extension... |
| [13:39:59] | <symbioquine[m]> | Am I missing something? |
| [13:43:03] | <symbioquine[m]> | Ah, I see |
| [13:43:20] | <symbioquine[m]> | It should be `->getModule($module)->hasImplementations(...)` |
| [13:43:35] | <mstenta[m]> | Oh... it was added in Drupal 9.4 |
| [13:44:08] | <symbioquine[m]> | symbioquine[m]: nvm, it's not that ^ |
| [13:44:46] | <mstenta[m]> | https://www.drupal.org/node/3000490 |
| [13:45:00] | <mstenta[m]> | You might still be on 9.3? |
| [13:46:03] | <symbioquine[m]> | Maybe something like;... (full message at <https://libera.ems.host/_matrix/media/r0/download/libera.chat/cf1a998128...) |
| [13:46:19] | <symbioquine[m]> | Would be backwards compatible (if that matters) |
| [13:46:26] | <mstenta[m]> | Yea that's the old way |
| [13:46:31] | <mstenta[m]> | Well farmOS `2.x` is on Drupal 9.4, so we don't need BC |
| [13:46:55] | <symbioquine[m]> | mstenta[m]: Yeah, I have an outstanding todo to get my dev environment updated 😒 |
| [13:47:30] | <symbioquine[m]> | Might have to bump that up the priorities... |
| [13:48:06] | <symbioquine[m]> | Was trying to get ready to inventory a whole bunch of seeds first, but it might actually be blocked on this |
| [13:48:08] | <mstenta[m]> | Maybe you can help me test the update hook instead |
| [13:48:27] | <mstenta[m]> | I don't think that will need `hasImplementations` because it will try to update ALL modules |
| [13:49:02] | <mstenta[m]> | Lemme see if I can give that a quick pass |
| [13:49:48] | <mstenta[m]> | FYI this is what I see after I install my `farm_bundle_test` module, which adds a `test_field` to activity logs |
| [13:49:49] | <mstenta[m]> | ACTION sent a code block: https://libera.ems.host/_matrix/media/r0/download/libera.chat/c7e54a8e49... |
| [13:50:04] | <symbioquine[m]> | > <@mstenta:matrix.org> ```... (full message at <https://libera.ems.host/_matrix/media/r0/download/libera.chat/96198a7bf9...) |
| [13:50:18] | <mstenta[m]> | oh will it? didn't think of that |
| [13:50:48] | <symbioquine[m]> | Yeah I think `\Drupal::service('entity_type.manager')->getDefinition($entity_type);` fails |
| [13:51:24] | <symbioquine[m]> | Gotta go feed the dog/chickens... back in a few |
| [13:51:45] | <mstenta[m]> | I'll add:... (full message at <https://libera.ems.host/_matrix/media/r0/download/libera.chat/ea8930d2e7...) |
| [13:53:00] | <mstenta[m]> | ah dang... well... uninstalling does not remove the field from the field map :-( |
| [13:53:07] | <mstenta[m]> | so we need that too |
| [14:53:12] | <symbioquine[m]> | Here's my working `hook_modules_installed`;... (full message at <https://libera.ems.host/_matrix/media/r0/download/libera.chat/5cdc6e62b7...) |
| [14:53:29] | <mstenta[m]> | Yea same |
| [14:53:58] | <mstenta[m]> | Testing now... |
| [14:53:59] | <mstenta[m]> | I'm actually just about done sketching up the uninstall bit too (generalized the logic into a helper that just swaps out the method call depending on install/uninstall) |
| [14:54:04] | <symbioquine[m]> | > <@mstenta:matrix.org> I'll add:... (full message at <https://libera.ems.host/_matrix/media/r0/download/libera.chat/aa91358563...) |
| [14:54:24] | <symbioquine[m]> | mstenta[m]: Awesome! |
| [14:54:26] | <mstenta[m]> | Oh really? 🤔 |
| [14:54:40] | <mstenta[m]> | Did you get an error? |
| [14:54:49] | <symbioquine[m]> | It wasn't returning null, it was throwing an error; |
| [14:54:55] | <symbioquine[m]> | ACTION uploaded an image: (6KiB) < https://libera.ems.host/_matrix/media/r0/download/matrix.org/BUgotFYEDSw... > |
| [14:55:11] | <symbioquine[m]> | Instead I ended up doing;... (full message at <https://libera.ems.host/_matrix/media/r0/download/libera.chat/6646787545...) |
| [14:55:15] | <mstenta[m]> | Oh your function is different than mine |
| [14:55:46] | <symbioquine[m]> | Could be another Drupal version difference? |
| [14:56:16] | <mstenta[m]> | `$entity_type_definition = \Drupal::service('entity_type.manager')->getDefinition($entity_type);` should return `NULL` if the type doesn't exist (I think) |
| [14:56:20] | <mstenta[m]> | did you find that was not the case? |
| [14:56:36] | <mstenta[m]> | or did you just refactor everything so it wasn't using `hasImplementations()` and so that didn't matter anymore? |
| [14:56:47] | <symbioquine[m]> | I think so, but can test again... |
| [14:57:03] | <symbioquine[m]> | mstenta[m]: I think that's a different issue |
| [14:57:36] | <mstenta[m]> | sorry i think we've forked a bit :-) |
| [14:57:55] | <mstenta[m]> | but it will be hard for you to test on 9.3 |
| [14:58:10] | <symbioquine[m]> | I'll get myself updated here shortly... |
| [14:58:24] | <mstenta[m]> | ok cool |
| [14:58:36] | <mstenta[m]> | i have to leave in a few minutes, but testing install/uninstall now... |
| [14:58:38] | <symbioquine[m]> | mstenta[m]: If I add `\Drupal::service('entity_type.manager')->getDefinition('plan');` near the top of that hook it fails like so; |
| [14:58:49] | <symbioquine[m]> | ACTION uploaded an image: (6KiB) < https://libera.ems.host/_matrix/media/r0/download/matrix.org/OOVVceZBBRi... > |
| [14:59:03] | <mstenta[m]> | oh ok |
| [14:59:05] | <mstenta[m]> | good to know |
| [14:59:13] | <symbioquine[m]> | If I comment it out, it works. |
| [14:59:25] | <mstenta[m]> | so then yea, i'll probably need to adapt to use your approach |
| [14:59:47] | <symbioquine[m]> | Unless that's also a Drupal 9.4 behavior difference |
| [14:59:51] | <mstenta[m]> | oh i doubt that |
| [14:59:51] | <symbioquine[m]> | In which case you could leave it |
| [14:59:54] | <symbioquine[m]> | k |
| [14:59:55] | <mstenta[m]> | i haven't tested without `plan` |
| [15:00:06] | <mstenta[m]> | but my uninstall test just failed :-( |
| [15:00:13] | <mstenta[m]> | gotta leave to get my son, will have to pause this |
| [15:00:24] | <mstenta[m]> | i think i know what the next steps are though |
| [15:00:30] | <mstenta[m]> | should have some time to work on it this weekend |
| [15:00:34] | <symbioquine[m]> | Awesome, thanks for the help on this! |
| [15:00:50] | <mstenta[m]> | yea! same to you! |
| [15:01:53] | <mstenta[m]> | i think we may need to replicate some of the logic in `onFieldDefinitionCreate()` and `onFieldDefinitionDelete()` but not all of it (just the field map stuff basically) |
| [15:02:20] | <mstenta[m]> | because it's trying to delete the field in `onFieldDefinitionDelete()` but can't find the table |
| [15:02:20] | <symbioquine[m]> | mstenta[m]: Hmmm |
| [15:02:27] | <mstenta[m]> | (since it's already been uninstalled) |
| [15:02:38] | <symbioquine[m]> | mstenta[m]: ah, bummer |
| [15:02:48] | <mstenta[m]> | so i think we just need a field map update method basically |
| [15:02:49] | <mstenta[m]> | more targetted |
| [15:03:01] | <mstenta[m]> | shame, was hoping we could just call the core code |
| [15:03:06] | <mstenta[m]> | but it's a bit intertwined |
| [15:03:17] | <symbioquine[m]> | Or contribute a patch that provides some way for it to work without the table there |
| [15:03:48] | <symbioquine[m]> | Seems pretty important for code to be able to clean up that field map even after a module is removed |
| [15:03:48] | <mstenta[m]> | perhaps... but then maybe we should juts contribute a patch to fix the overall issue |
| [15:04:07] | <symbioquine[m]> | haha yeah, slippery slope argument invoked |
| [15:04:14] | <mstenta[m]> | haha yea |
| [15:04:24] | <mstenta[m]> | ok well until next time! |
| [19:11:53] | <mstenta[m]> | symbioquine: jeez i dunno... maybe it is worth a drupal core patch 😬 |
| [19:12:44] | <mstenta[m]> | i get nervous copying all this logic for maintaining the field maps... if drupal core ever changes it for whatever reason, and we don't follow suit, it could break |
| [19:15:26] | <symbioquine[m]> | Yeah |
| [19:15:40] | <mstenta[m]> | Reading the core issue in more detail now... |
| [19:15:59] | <mstenta[m]> | But at least we're on the right track I think :-) |
| [19:16:01] | <symbioquine[m]> | Especially as it pertains to that caching |
| [19:17:05] | <mstenta[m]> | It's interesting... I'm curious what else the field maps are used for |
| [19:17:23] | <mstenta[m]> | Surprised we didn't notice this earlier |
| [19:18:05] | <mstenta[m]> | But I guess it's just not used for most normal bundle field logic (UI, Views, etc all work fine) |
| [19:23:29] | <mstenta[m]> | I think the issue makes sense... I'm adding a comment now |
| [19:23:39] | <mstenta[m]> | A patch might not be too hard actually... |
| [19:24:32] | <mstenta[m]> | The bigger question is what's the "right" way to solve it. @joachim suggested one, but it feels a little ugly. It should work effectively in theory though. |
| [19:24:47] | <mstenta[m]> | This bit: |
| [19:24:47] | <mstenta[m]> | > I guess we could get a list of each implementation from the module manager, and skip field module's implementation? That feels pretty ugly though. |
| [19:25:05] | <mstenta[m]> | I'm summarizing better in my comment... standby :-) |
