| [08:25:04] | <mstenta[m]> | paul121: your patch to the `entity` module has been merged! Only took 4 years! ð
https://www.drupal.org/project/entity/issues/3190436 |
| [08:25:44] | <mstenta[m]> | That was the only patch we were applying to entity, so we can stop pinning that module now! |
| [08:25:53] | <mstenta[m]> | ð |
| [08:30:46] | <mstenta[m]> | Opened a PR to run tests...https://github.com/farmOS/farmOS/pull/867 |
| [09:57:21] | <symbioquine[m]> | https://github.com/farmOS/farmOS/pull/866 and https://github.com/farmOS/farmOS/pull/864 should be ready for review ð
|
| [10:14:21] | <symbioquine[m]> | <mstenta[m]> "paul121: your patch to the `..." <- It looks like the diff that got merged isn't the same as the one we've been patching with... |
| [10:14:36] | <symbioquine[m]> | https://git.drupalcode.org/project/entity/-/commit/a64d88644639a580cff7b... vs https://www.drupal.org/files/issues/2022-05-11/3206703-10.patch |
| [10:16:04] | <symbioquine[m]> | ACTION uploaded an image: (226KiB) < https://matrix.org/_matrix/media/v3/download/matrix.org/KUZvKQYIIrhbMUuE... > |
| [10:16:26] | <symbioquine[m]> | That highlighted bit isn't present in what got merged. |
| [10:16:49] | <symbioquine[m]> | (I'm not totally sure I grok the consequences of that, just pointing it out.) |
| [10:40:47] | <mstenta[m]> | Ugh it looks like the MR was merged, but not Paul's patch?? |
| [10:41:46] | <symbioquine[m]> | Also, maybe you can educate me as to why only some of the patches are listed in the issue... |
| [10:41:50] | <symbioquine[m]> | https://www.drupal.org/project/entity/issues/3190436 |
| [10:41:56] | <mstenta[m]> | I just noticed that too |
| [10:42:11] | <symbioquine[m]> | Specifically, I don't see the one that we've been using with the "-10.patch" suffix. |
| [10:42:53] | <mstenta[m]> | Oh wait a minute... that's not the issue we're patching! |
| [10:42:54] | <mstenta[m]> | https://www.drupal.org/node/3206703 |
| [10:43:46] | <mstenta[m]> | Ah that's my bad. I guess I was too overjoyed. |
| [10:44:56] | <mstenta[m]> | I got an email notification that the other had been fixed, and didn't look closely. I guess we aren't using that patch, but I was still subscribed to the issue. |
| [10:45:09] | <symbioquine[m]> | I didn't even look at the number. Oops. ð
|
| [10:45:17] | <mstenta[m]> | Haha oh well. I'll close the PR. |
| [10:45:25] | <mstenta[m]> | I'm curious though, because it seems like there is some overlap |
| [10:45:33] | <symbioquine[m]> | Agreed |
| [10:45:35] | <mstenta[m]> | So we might still need to update our patch |
| [10:45:47] | <mstenta[m]> | But maybe that just simplifies it |
| [10:46:59] | <symbioquine[m]> | Well, the other line also is missing the ` && !empty($this->tableMapping->getAllFieldTableNames($field->getName()))` part... |
| [10:47:31] | <symbioquine[m]> | So the (new) diff would still need to modify that line (I assume). |
| [10:48:08] | <mstenta[m]> | Thanks for checking symbioquine |
| [10:48:11] | <symbioquine[m]> | s/other/earlier/ |
| [10:48:19] | <symbioquine[m]> | np |
| [10:48:39] | <symbioquine[m]> | Was mostly just curious. Hopefully the tests would have also caught it. |
| [10:49:16] | <symbioquine[m]> | symbioquine[m]: Actually, I guess not. Seems like they're basically passing. |
| [10:50:01] | <mstenta[m]> | Guess we aren't testing for it |
| [10:50:54] | <mstenta[m]> | Patching upstream like this is sort of a fuzzy area when it comes to test policy IMO |
| [10:51:07] | <mstenta[m]> | Ideally, we shouldn't have to test upstream code |
| [10:51:24] | <mstenta[m]> | But in practice this kind of thing happens |
| [10:52:03] | <mstenta[m]> | Otherwise, where do you draw the line? Should we be testing that Drupal core provides an "entity" concept? ðĪŠ |
| [10:53:36] | <symbioquine[m]> | We can rely on best intentions to manually validate that our upstream does what we expect or we can write defensive tests to confirm (at least smoke test) that it does. |
| [10:53:44] | <mstenta[m]> | Yea agreed. And in this case we would probably catch it if we were testing our Views better. |
| [10:53:45] | <mstenta[m]> | That's something we are lacking generally right now... Views tests |
| [10:53:48] | <symbioquine[m]> | In practice a lot of that smoke testing gets covered by our main tests, but some cases like this - where we're perhaps relying on fringe functionality might deserve special attention. |
| [10:54:10] | <symbioquine[m]> | (I assume entity reference bundle fields might be fringe functionality, but I actually don't know how other folks use Drupal very much ð
) |
| [10:55:52] | <mstenta[m]> | Yea the reverse relationship functionality could be considered "fringe", since it seems we are the only ones who requested it, and are the only ones who have commented on the issue ð
https://www.drupal.org/node/3206703 |
| [10:56:51] | <mstenta[m]> | Ahhh reading further... paul121 explains this already in his comment: |
| [10:56:51] | <mstenta[m]> | > Adding a patch that includes the change from #3190436: EntityViewsData Column information not available for computed entity reference fields. The two patches are conflicting and we need both of them for farmOS |
| [10:56:59] | <mstenta[m]> | https://www.drupal.org/project/entity/issues/3206703#comment-14056032 |
| [10:57:09] | <mstenta[m]> | So basically we were getting a two-for-one patch ð |
| [10:57:40] | <mstenta[m]> | Fair enough! So now we just need to tease them apart since one was merged. |
| [14:22:11] | <paul121[m]> | Good digging... this has prob left my brain at this point. TY for the hive-mind ð ð§ |
| [15:18:16] | * farmBOT has joined #farmos |
