Closed (fixed)
Project:
Rules
Version:
8.x-3.x-dev
Component:
Rules Core
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
17 Oct 2019 at 04:20 UTC
Updated:
7 Aug 2020 at 08:49 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
tr commentedChanged title.
Comment #3
tr commentedAdding a parent issue so we can more easily find these issues that are deferred until core 8.8.
Comment #4
jonathan1055 commentedThanks for adding to the parent.
The newer issue I had created #3101026: Replace deprecated path.alias_manager and AliasManager when Drupal 8.8 becomes the lowest supported version half overlaps with this one. We can address that later when we come to work on the fix.
Comment #5
tr commentedComment #6
tr commentedWell, that was a major pain in the butt. Let's see if the testbot approves.
Comment #7
tr commentedAside from the one coding standards mistake everything's passing.
The question I have is what should we do about passing aliases in the context.
When aliases were just string, it made sense to pass around string.
Now that they're entities, it would be more powerful to pass around the complete entity, not just the string. Context data already has the alias entities (e.g. node.path.alias, which is an entity not a string). This makes it harder to create new aliases in Rules, though, since we would have to do a "Create entity" action followed by a "Create path alias" action. Similar to how Roles are difficult to work with in Rules...
If you look at the patch, I did change one condition to use the path_alias entity instead of a string - that's "Path alias exists". I'm not sure that's the right thing to do, and I'm not sure if we should do it everywhere. Perhaps just leave the context values as strings and open a new issue to consider changing them to entities on a case-by-case basis? I will have to play around with this patch for a while and try to build some rules and see how it goes.
Comment #8
jonathan1055 commentedGood work, yes it did look a PITA.
Your idea of leaving the context values as strings for now is fine. We can open new issues to change them later if necessary.
Comment #9
tr commentedYeah, and when I think about it it doesn't even make sense to pass a path_alias entity to "Path alias exists", because if you have the entity, then it obviously does exist and the condition will always return true. The real question we want to ask is "does a path_alias entity exist with the given string alias", and for that it makes sense to continue doing things the way we were.
I'm not ruling out using the path_alias entities directly - I think we will need to so this eventually, but that will be a different issue.
Comment #10
jonathan1055 commentedPatch #9 work nicely. On Travis it eliminated completely the deprecation message
16x: \Drupal\Core\Path\AliasStorage is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0and we just have the one remaining deprecation message. The allowed count needs to be increased though, as this deprecation count was 4045 but is now 4229, due to the extra bits in the tests in this patch.Patch #10 just adds the change to .travis.yml, nothing else is altered in the real php code.
Comment #12
tr commentedOK, I added the changes to .travis.yml and committed.
Comment #13
jonathan1055 commentedThanks. Yes the Travis tests now show only the one remaining deprecation message.