| [11:14:28] | <symbioquine[m]> | I'm trying to figure out where the logic that drops the "Something: " prefix from assets names that appear in movement log names is implemented; |
| [11:14:35] | <symbioquine[m]> | ACTION uploaded an image: (116KiB) < https://libera.ems.host/_matrix/media/r0/download/matrix.org/OqLyySDRolo... > |
| [11:15:02] | <symbioquine[m]> | It doesn't seem to be [here](https://github.com/farmOS/farmOS/blob/04bf771d91cde8276b67090d2b97c4e455...) |
| [11:15:18] | <symbioquine[m]> | or [here](https://github.com/farmOS/farmOS/blob/04bf771d91cde8276b67090d2b97c4e455...) |
| [11:15:57] | <symbioquine[m]> | But naively, we'd imagine the log name would be `"Move Tractor: Slow Red to Big Field A, Big Field B"` |
| [11:19:35] | <symbioquine[m]> | It actually seems like pretty decent/sane behavior, but it feels a bit towards the "conventions" side of things... |
| [11:20:55] | <symbioquine[m]> | i.e. how does someone choosing their conventions know that farmOS provides that "feature" if they name their assets with the "Something: " prefix pattern? |
| [11:31:08] | <mstenta[m]> | symbioquine: is this what you're looking for? https://github.com/farmOS/farmOS/blob/81da2ed9b3fcc8dacb23fadbe72b85cdc5... |
| [11:31:18] | <mstenta[m]> | `$log_name = $this->t('Move :assets to :locations', [':assets' => $asset_names, ':locations' => $location_names]);` |
| [11:32:16] | <mstenta[m]> | oh wait no |
| [11:32:19] | <mstenta[m]> | that's what you linked to above |
| [11:32:25] | <symbioquine[m]> | Maybe. Does Drupal do some magic when doing those string substitutions? |
| [11:32:30] | <symbioquine[m]> | That would seem surprising. |
| [11:32:36] | <mstenta[m]> | i'm not sure i follow |
| [11:32:58] | <mstenta[m]> | oooooh i see what you mean! |
| [11:33:03] | <mstenta[m]> | that is a bug! |
| [11:33:11] | <mstenta[m]> | i think paul121 actually just discovered this in another context recently! |
| [11:33:35] | <mstenta[m]> | yes, we need to change that line (that i pasted above) so that it uses `@` instead of `:` for the placeholdres |
| [11:33:55] | <symbioquine[m]> | mstenta[m]: Interesting! |
| [11:33:56] | <mstenta[m]> | care to open a PR? :-) |
| [11:34:32] | <mstenta[m]> | i forget exactly why, but we realized this recently... paul121 may have actually found this one too - i think he did a grep for other occurrences so we could fix all of them |
| [11:34:45] | <symbioquine[m]> | Sure, I can do that... |
| [11:34:59] | <symbioquine[m]> | Is there an issue already? |
| [11:35:09] | <mstenta[m]> | i don't think so |
| [11:35:24] | <mstenta[m]> | https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Component%21Rend... |
| [11:35:44] | <mstenta[m]> | describes the different filtering that takes place with `@` vs `:` |
| [11:36:01] | <mstenta[m]> | > `:variable`: Return value is escaped with \Drupal\Component\Utility\Html::escape() and filtered for dangerous protocols using UrlHelper::stripDangerousProtocols(). Use this when using the "href" attribute, ensuring the attribute value is always wrapped in quotes: |
| [11:36:27] | <mstenta[m]> | that should only be used in `href`... not in normal strings (eg log names) |
| [11:36:41] | <symbioquine[m]> | hahaha, yeah I see how it's definitely a bug in this context |
| [11:36:46] | <symbioquine[m]> | If we wanted that behavior, we wouldn't do it that way :) |
| [11:36:53] | <mstenta[m]> | yup |
| [11:37:10] | <mstenta[m]> | just fat fingers / laziness / forgetfulness about the differences |
| [11:37:55] | <mstenta[m]> | (i've made this mistake before... i'm usually careful about it... guess this one slipped through) |
| [11:39:02] | <mstenta[m]> | paul121: do you remember talking about this recently? where was the one you discovered, do you recall? and did you find others? |
| [11:39:06] | <symbioquine[m]> | Cool, maybe there's a way we can add a code sniffer style rule about it... |
| [11:39:13] | <mstenta[m]> | yea |
| [11:39:26] | <mstenta[m]> | surprised there isn't already! |
| [11:39:35] | <mstenta[m]> | maybe it's hard to scan for |
| [11:39:45] | <mstenta[m]> | 🤷 |
| [11:40:05] | <symbioquine[m]> | might be that |
| [11:59:38] | <paul121[m]> | <mstenta[m]> "paul121: do you remember talking..." <- yup! when you use the `:` placeholder it expects that you are giving it a URL and it does some logic to remove everything before the last `:`. I forget why |
| [12:00:10] | <symbioquine[m]> | <mstenta[m]> "> `:variable`: Return value is..." <- Vaguely this is why ^ |
| [12:00:52] | <paul121[m]> | yea, I guess that would be hard because codesniffer needs to understand the context for the parameter |
| [12:01:40] | <paul121[m]> | normally '@` would be used like this: `$this-t('Here is a <a href="@url">link</a>', ['@url' => 'https://farmos.org']);` |
| [12:01:50] | <paul121[m]> | * normally `@` would be used like this:`$this-t('Here is a \<a href="@url">link\</a>', \['@url' => 'https://farmos.org'\]);\` |
| [12:02:29] | <paul121[m]> | * normally `@` would be used like this:`$this-t('Here is a <a href="@url">link</a>', \['@url' => 'https://farmos.org'\]);\` |
| [12:03:10] | <paul121[m]> | but you might use it in other ways as well 🤷 |
| [12:03:50] | <paul121[m]> | <mstenta[m]> "i forget exactly why, but we..." <- our bug was in the Rothamsted code :-) |
| [12:04:45] | <symbioquine[m]> | paul121[m]: But is there a scenario is farmOS where we'd want that escaping behavior? |
| [12:06:08] | <symbioquine[m]> | I think it would be much easier to have a check for any use of the colon substitution patterns. |
| [12:07:20] | <paul121[m]> | * normally `:` would be used like this:`$this-t('Here is a <a href=":url">link</a>', \[':url' => 'https://farmos.org'\]);\` |
| [12:07:29] | <paul121[m]> | oh oops I had it backwards |
| [12:09:23] | <paul121[m]> | yeah good question |
| [12:09:43] | <paul121[m]> | can we always expect the `:` replacement to be within an anchor tag? |
| [12:12:21] | <paul121[m]> | I did a quick search... that seems to be the case |
| [12:12:44] | <paul121[m]> | I found a few other instances in the planting quick form: https://github.com/farmOS/farmOS/blob/81da2ed9b3fcc8dacb23fadbe72b85cdc5... |
| [12:34:25] | <symbioquine[m]> | <paul121[m]> "can we always expect the..." <- I'm not sure the detection logic would need to be that context aware. If we can say that code in the farmOS code-base should almost always use `@` substitutions, then we could just use `grep` to do the detection and maybe check the preceding line for a pragma like; |
| [12:34:26] | <symbioquine[m]> | ```php |
| [12:34:26] | <symbioquine[m]> | //# next-line-intentional-use-of-colon-translation-substitution |
| [12:34:26] | <symbioquine[m]> | ``` |
| [12:42:51] | <paul121[m]> | oh true! |
| [12:44:04] | <mstenta[m]> | Is there any detection already to ensure that hrefs use a colon? |
| [12:44:18] | <mstenta[m]> | That is the more critical thing from a security perspective |
| [12:44:46] | <paul121[m]> | right now I use the same farmOS 2.x-dev image & phpcs rules in my contrib module template. for better or worse, this would cascade to those repos as well.... it would be nice if the change didn't require adding the comment, but doesn't mean I'm totally against that :-) |
| [12:45:21] | <mstenta[m]> | Should check to see if this has been discussed upstream |
| [12:45:26] | <mstenta[m]> | I have to imagine it has |
| [13:27:48] | <symbioquine[m]> | https://farmos.discourse.group/t/asset-link-dev-log/1175 🎉 |
| [18:24:47] | * EvanKelley[m] has quit (Ping timeout: 240 seconds) |
| [18:37:06] | * EvanKelley[m] has joined #farmos |
