| [10:13:49] | <symbioquine[m]> | <mstenta[m]> "And the third: https://github...." <- Are those second two supposed to still be drafts? |
| [10:15:50] | <symbioquine[m]> | Also I wonder is somebody like pcambra would be a better reviewer... I can skim them, but I'm not sure I read enough PHP to have much chance of catching minor issues/anomalies. |
| [10:26:27] | <mstenta[m]> | I changed them to "Ready for review" |
| [10:27:12] | <mstenta[m]> | Yea I would welcome reviews from anyone who is willing and able! cc pcambra wotnak (other Drupal pros in here?) |
| [10:27:26] | <mstenta[m]> | paul121 said he will take a look soon too |
| [11:40:17] | * farmBOT has joined #farmos |
| [13:11:30] | * pcambra[m] has joined #farmos |
| [13:11:31] | <pcambra[m]> | in one go? +10,221 −11,536 |
| [13:11:43] | <mstenta[m]> | sorry |
| [13:12:19] | <mstenta[m]> | honestly I'm not sure how realistic it is to review in detail |
| [13:12:41] | <mstenta[m]> | especially the hooks one |
| [13:12:58] | <mstenta[m]> | the other two are a bit easier (the autowiring one is the simplest) |
| [13:14:28] | <mstenta[m]> | the hooks one is really just moving code around - no major changes |
| [13:14:32] | <mstenta[m]> | (hence the +10,221 −11,536) |
| [13:14:34] | <pcambra[m]> | Impossible 😅, I'd recommend getting this in 3+ subsequent PRs |
| [13:14:38] | <pcambra[m]> | stuff like modules/asset/group/src/GroupMembership.php can go in independently from the hooks |
| [13:14:44] | <pcambra[m]> | or the composer.json change? |
| [13:15:18] | <mstenta[m]> | oh... important to note: that hooks one includes all the commits from the other two |
| [13:15:24] | <pcambra[m]> | maybe committing those would make the hooks more palatable :D |
| [13:15:31] | <pcambra[m]> | yeah, sorry that's what I meant |
| [13:15:38] | <mstenta[m]> | this is a better comparison: https://github.com/mstenta/farmOS/compare/4.x-autowire...4.x-hooks |
| [13:16:03] | <mstenta[m]> | > 9,196 additions and 7,153 deletions. |
| [13:16:03] | <mstenta[m]> | a little better 😅 |
| [13:16:20] | <pcambra[m]> | is this all rector? |
| [13:16:49] | <mstenta[m]> | not all no - but i did start with recotr |
| [13:17:02] | <mstenta[m]> | i would recommend reading the commit messages in order to get a sense |
| [13:19:32] | <pcambra[m]> | maybe autowiring could be another one |
| [13:19:45] | <mstenta[m]> | yes these are all separate PRs |
| [13:19:48] | <mstenta[m]> | https://github.com/farmOS/farmOS/pull/1005 |
| [13:19:51] | <mstenta[m]> | https://github.com/farmOS/farmOS/pull/1006 |
| [13:19:55] | <mstenta[m]> | https://github.com/farmOS/farmOS/pull/1007 |
| [13:20:08] | <mstenta[m]> | they just build on one another, so the latter ones include commits from the former ones |
| [13:22:25] | <mstenta[m]> | i should say: i also did extensive manual testing of all major features in the UI after these changes... so I am fairly confident with them - but of course it's possible small bugs may sneak in - I figure if there are any we can fix them as they come up and add tests to make sure they are covered in the future |
| [13:23:07] | <mstenta[m]> | (i'm so grateful for the coverage we have! wouldn't have been able to do all this without it! |
| [13:24:44] | <pcambra[m]> | wondering why this is needed? #[Autowire(service: 'serializer')] |
| [13:26:22] | <mstenta[m]> | If I remember correctly there were some core services that didn't have aliases for autowiring |
| [13:26:33] | <mstenta[m]> | (Where was that one, so I can look at it specifically?) |
| [13:28:18] | <pcambra[m]> | I've added a comment in one, but I've found a few, i.e. |
| [13:28:19] | <pcambra[m]> | modules/core/import/modules/csv/src/Controller/CsvImportController.php |
| [13:29:34] | <mstenta[m]> | Cool I will respond to comments in the PR |
| [13:29:37] | <mstenta[m]> | Thanks pcambra ! |
| [13:30:34] | <pcambra[m]> | I don't see anything strange, I just added a few minor questions |
| [13:32:32] | <mstenta[m]> | Responded to your questions |