Path aliases are no longer arrays and cannot pass additional data to path hooks

Talked with davereid in irc about this and I do like the $source, $alias, $langcode pattern. The best/easiest solution I see at the moment is passing an additional $options array. That array will be passed to the hooks.

Lets not shout and bugs that recur are only kind of special. Lets use a tag for that. The title is already quite clear that past behavior was different (and better).

I think I commented along these lines in the original issue already, but here we go again:

So, the obvious quick-fix would be to simply convert this back into an array. I'd +1 that, since the current API is a clear regression in terms of DX (I worked against it already, so this is not unfounded).

However, the second best option would be to go straight to typed Path\Alias class objects instead. That would give properly defined + documented path alias properties on every object.

Off-hand, I cannot see which (contrib) use-case would not be able to work with object properties; i.e., instead of setting an array key, you simply set an object property, and/but like before, any additional property is not persistent and only exists for the lifetime of the path alias variable.

What do you think?

I'd be happy to work on this. So our options are either:
1. Passing in a hashed array (this is compatible with previous versions)
2. Passing in an extra 'options' array to go for module hooks

I'm +1 for option (1) too.

P.S. @sun it seemed like you were also suggesting that instead of passing a hashed array (as specified in (1) ) it should accept an object and use it's properties although that would break compatibility again, wouldn't it?

@Crell in #1269742-22: Make path lookup code into a pluggable class where the change away from an array was first suggested:

+++ b/core/lib/Drupal/Core/Path/DrupalPathRegistry.php
@@ -0,0 +1,102 @@
+  public function save(array &$path) {

I know this is a straight-port, but I want to try and avoid anonymous arrays as data objects. If Path is not going to be its own data object, then the CRUD functions should take multiple useful parameters rather than an anonymous data array.

I generally would agree with that assessment and would prefer either explicit arguments or a classed object.

Finally looking to start contributing to D8, the path API seems a relatively familiar place to start;). Here's a shot at adding an options array to Path::save().

Moved to path.module per #2050763-16: Refine "base system" component (notes on refactoring of "base system" category here: https://docs.google.com/a/acquia.com/spreadsheet/ccc?key=0AusehVccVSq2dF...).

8: path-extra_data-1854284-8.patch queued for re-testing.

Re-roll.

We're porting pathauto and redirect to 8.x and been trying find workaround for this, but it's a mess as we have to resort to global state. So this is still a problematic regression.

While I agree that path should probably be a value object, adding a separate $options argument is only an API additional and possibly the easiest option at this point.

Tagging, thanks @xjm.

Honestly, throwing an extra "random crap goes here" array in seems like going backward.

Why not go forward? Go all the way to a classed Path object, which Pathauto can subclass if it wants to pass around extra data?

Crell: Yeah, that would be nice, but that's quite a change (I found ~20 calls to save/delete/load, new classes, lots of test calls). And it is getting very very late in the cycle.

Discussed this a bit with @dawehner. Is there any other real use case other than the original thing? What if we simply make this a default feature? Just like entity update hooks, we by default load the original data and pass it along to the hook. When a conversion to an Alias (because it would be that, not path ;)) class, we can easily change that to getOriginalAlias() that would return the old alias object.

If so, then this is much easier to do.

I agree that passing along some options is kinda of a bad idea. I mean people could come up with examples for that in pretty much every other
part of core. I think we should try to avoid that.

On the other hand to convert it to an object is a non-trivial amount of effort, especially because it is an API change, which would need another
big amount of time, to finish it properly. If someone though is motivated to do so, it can be still done after this patch.

The change from berdir makes sense, given that there are many update implementation which return some context of the previous value, like
for example \Drupal\Core\Database\Query\Update::execute() with the amount of updated rows.

+++ b/core/lib/Drupal/Core/Path/AliasStorage.php
@@ -63,11 +63,19 @@ public function save($source, $alias, $langcode = LanguageInterface::LANGCODE_NO
+      $original = $this->connection->select('url_alias')
+        ->fields('url_alias', array_keys($fields))
+        ->condition('pid', $pid)
+        ->execute()
+        ->fetchAssoc();

Let's just use a static query.

Changed it to a static query.

Will update the issue if others agree with this as a solution.

Yes, it doesn't, but on the plus side, that will make it work for all kinds of updated aliases. We can also move that to a separate issue.

The problem here I see is that an additional $options array doesn't seem likely to get accepted, going back to an array even less, and going forward to an object would be quite the change and I don't know if it would be accepted and if someone has time to work on that.

I agree with @Crell. Adding options array where where throw everything that we don't know where to put isn't the best solution. Current patch is IMO a step in right direction.

It would be great if we could proceed with conversion to actual path object. Could we target 8.1 with that?

I'm fairly certain converting to an Alias object would be a breaking API change, so not allowed in 8.1. It would have to get in before that. We won't know how hard it is unless we try, though. :-)

Let's RTBC this and open another issue for object conversion and see if we're capable of doing it. How does that sound?

Updated the issue summary with all discussed options, also created a change record draft: http://drupal.org/node/2336611

That seems like a reasonable plan, Berdir.

Dave: "Place to dump random arbitrary data for whatever" is not a pattern that can work in modern high-end software (or low-end, for that matter). We need alternatives to that. Providing a "previous version" object is a structured and consistent-with-other-subsystems way of addressing some use cases. That's a step forward. If you have structured (ie, non-"big array of extra stuff") recommendations for how else to improve the API here to make it more flexible please share them.

Committed c78a5c2 and pushed to 8.0.x. Thanks!

This seems a good place to start. Yes #2224761: Add a generic way to add third party configuration on configuration entities and implement for field configuration added a way to store third party data but it was not arbitrary. It's using a interface to control how it is stored on the object and made it play nice with config schema. Hopefully #2336597: Convert path aliases to full featured entities will create a nice way for modules to add additional data to paths.