Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
In Drupal\node\Plugin\views\row\Rss::render:
$node->link = $node->toUrl('canonical', ['absolute' => TRUE])->toString();
Similar code exists in comment too.
If you have a field called link this will set $node->link->uri and do nothing else. Later
$item->link = $node->link;
Now clearly this wanted to put a string in there but instead this is a field item list and TwigExtension throws a fatal.
Steps to reproduce
- Remove the
field_prefix - Add a field called
link - Add a node
- Create an RSS view
- Watch the WSOD
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | 3218024_7.patch | 1.77 KB | Ghost of Drupal Past |
Comments
Comment #2
Ghost of Drupal PastComment #3
longwaveWhy does
$node->linkneed to be set at all, can't it be set directly on$item?Comment #4
Ghost of Drupal PastThe problem is backwards compatibility: it's not impossible during the entity build process someone changes $node->link .
Now that I slept on it, fixing this is not that hard, just need to be explicit about it. After all, if some code does a $node->link = 'foo' during entity build that also just changes $node->link->uri much the same as the core code does so just retrieve that.
Comment #5
BerdirThat does seem like a weird fix, because the code is going to mess up your link field value. A few lines above, there is:
$node->link = $node->toUrl('canonical', ['absolute' => TRUE])->toString();
Honestly, what this is doing seems completely bogus.
Why set $node->link when the same function just a few lines down just fetches it again. It *does* pass the node through entity view builder inbetween, but I struggle to see why anyone would want to do something with $node->link during that time. Change the link target? They could do that in preprocess.
As far as I see, $node isn't even available through $item or the template later on, just the node ID.
Personally, I'd be happy to define that the behavior of arbitrary stuff that's put on nodes is not covered by BC, although we do have some very frequently used ones, like original or in_preview, we can make exceptions there.
So we could just switch this out to $item->link = $node->toUrl('canonical', ['absolute' => TRUE])->toString(); IMHO. We could even for BC check for $node->link being set and not being a field and then use that, together with a deprecation message. the only thing that would break is then trying to read that property during a hook_entity_view(). There might be ways to deal that too, e.g. set the property if not defined as a field (if that happens, the code would break anyway) and if it was changed, trigger a deprecation. But as mentioned, perfectly happy to not bother with that.
I can see that there are a handful of modules that use rss_elements (http://grep.xnddx.ru/search?text=-%3Erss_elements), I see the use case for that, but link?
I'd love to deprecate the ability to just put random stuff on entity objects completely as well a deprecating __get/__set, we have issues to instead have some kind of temporary storage that's explicit about it.
Comment #6
longwaveI traced the origin of this code back to the very first version of Views for D7, it was added while porting Views from D6.
I think this is just a historical artifact and agree with @Berdir that this shouldn't be relied upon as an internal implementation detail of RSS output; as stated if someone wants to change the link they should probably be doing it in preprocess anyway.
Comment #7
Ghost of Drupal PastTo be fair, if not for BC this is the only sensible fix because if there is a field called link then assigning $node->link actually loses user entered data.
Comment #8
longwave#7 is the simplest way to solve this issue. I checked the BC policy and the closest thing I can find is:
which to me means I think we are OK to make this change.
Comment #11
catchYes #7 looks fine to me as well and #8 is correct on the bc policy.
Committed/pushed to 9.3.x and cherry-picked to 9.2.x, thanks!