🇬🇧United Kingdom @alexpott

alexpott → created an issue.

OK, fixing all issues on level 6 is more work than I anticipated, so I'm adding a bunch of things to the baseline.

Yeah that's why I stopped after doing the simple stuff in tests :D...

It is hard when core's docs don't meet the requirements. And yeah adding to the baseline is a great way forward.

I think we could just log the current value. However I also think there is a way around #60.1 If we want to also report the initial value then we could easily do something like

if (!isset($this->keySetDuringRequest[$key])) {
  $this->keySetDuringRequest[$key] = $value;
}

And then the method to get the keys set during the request could return the initial value too. I think we should only consider doing that if getting the initial value does not cost anything. I think it is okay to ignore multiple changes to a value during a request and only care about the initial and value value for the purposes of logging.

I think we should consider an architecture that allows us to log state keys set during a request / cli operation. We could add an property to store the keys that are set during the request to \Drupal\Core\State\State and then add a public method to get this information. Then add a service then subscribes to the terminate even and gets this value and logs any state changes for a list of keys defined in a container parameter. This container parameter should contain the value system.maintenance_mode be default.

This way we'll get consistent logging across Drush / admin UI and any other way of doing this and we'll have a generic system to solve this if we want logging for any other state changes.

Minor fixed needed to pass PHPStan checking...

I've rewritten the CR and published it and I've added a note to the original CR https://www.drupal.org/node/3440505 → that it is no longer valid.

@heikkiy I think the premise of ✨ Use CDATA in XML RSS Feeds (was: Malformed XML Feeds) Closed: outdated was probably valid when it was filed - back in 2003!!! - but the code in views and use of views for core's RSS feed came later and had an alternate fix - escaping the html. Unfortunately 3433 remained open and people continued to work on it without validating that the fix was still necessary although there definitely is discussion of this on the issue. Also it appears that what RSS validors say and RSS readers do is different. Note that https://www.rssboard.org/rss-encoding-examples is quite clear that escaped HTML tags in the description field are supposed to work and be respected and this is was working well prior to 10.3.

@heikkiy we're going to remove the cdata tags because they are not necessary. We're already escaping the html in RSS description fields which as per #43 is a supported way of embedding HTML in there. The whole cdata approach appears to not have been necessary. Do you have needs or information that says it is necessary?

Removed the update function. Back to RTBC as this is quite a simple change.

alexpott → created an issue.

Removing the "needs tests" added in #11 because everything I've added since then has test coverage and there was no additional test coverage requested by #11.

I'd done some more thinking about #11.2 I agree that there is an issue with information going out of date when you create a redirect from a path alias as in the situation described. But; this is not caused by redirects per se. You can create exactly the same situation without them. For example:

1. Install the standard profile.
2. Install entity usage and configure the usage tab to appear on content
3. Create node 1 and alias it to '/foo'
4. Create node 2 and in the content add a link to '/foo'
5. Edit node 1 and change the alias to '/foobar'
6. Go to the usage tab of node 1 see that the usage shows node 2... even though the link is now broken and does not point to the entity.
7. Create node 3 and alias it '/foo'.
8. Go to node 3 usage - shows nothing
9. Go to node 1 usage - still shows node 2...
10. Go to node 2 and edit and save... things update...

The point being we need to update usage when aliasing changes too. I think we should think about fixing all of this in a separate issue - that is likely to get quite complex.

@dhruv.mittal you need to update the MR branch with the latest changes from 8.x-2-x... the MR fork is very old.

Added the code and test to address #11.1

Re #11.1 Yes this makes sense. Implementing a solution and will update once ready and tested.

I think I disagree with #11.2 - redirects are not really bridges - they are managed outside of the entity and have their own UI and knowing that a redirect is pointing to an entity is useful information. Still mulling this over though. I think maybe the example you given means not that we need to remove redirects but that we need to provide more info on redirects... complex.

alexpott → created an issue.

alexpott → created an issue.

alexpott → created an issue.

alexpott → created an issue.

Perfect.

I think theoretically you could implement any entity tracking plugin that does not use fields BUT the entity track plugin definition has field_types so it is going to be quite hard.

But this potentially would break such a use-case. Maybe the way around this is to look at all the entity track plugins and check to see if the extend from \Drupal\entity_usage\EntityUsageTrackBase - if they all do then there we should not include non-fiedldable types. This would improve the UI for 99.5% of users.

+++ b/src/Form/EntityUsageSettingsForm.php
@@ -113,8 +114,13 @@ class EntityUsageSettingsForm extends ConfigFormBase {
+      // If the entity type doesn't have fields it can't be a source, e.g. views
+      if (method_exists($entity_type->getOriginalClass(), 'getFields')) {
+        $source_options[$entity_type->id()] = $entity_type->getLabel();

I think we should be checking FieldableEntityInterface so something like:

      if ($entity_type->entityClassImplements(FieldableEntityInterface::class)) {
        $source_options[$entity_type->id()] = $entity_type->getLabel();
      }

You will need to add a use statement too.

FWIW I think this is a bug.

Very nice - I think with the addition of a CR we are good to go here.

Committed and pushed 6c01bb31917 to 11.x and f86f182a989 to 11.1.x. Thanks!

Committed and pushed 9e7842eb89c to 11.x and 045cc0e43f6 to 11.1.x. Thanks!

This is ready. Once this is in I'll open a follow-up to get to level 6 and then we'll have return typehint checking ftw.

I would love to get 📌 Manage user session with jwt Postponed in

Plus I think we should increase the PHPStan level and add return types and parameter types everywhere.

alexpott → created an issue.

1. +++ b/src/Plugin/EntityUsage/Track/ParagraphsEntityEmbed.php
   @@ -0,0 +1,63 @@
   +  public function parseEntitiesFromText($text) {
   +    $dom = Html::load($text);
   

   Before you do any parsing here you should check that embedded_paragraphs are in the target entity types.

2. +++ b/src/Plugin/EntityUsage/Track/ParagraphsEntityEmbed.php
   @@ -0,0 +1,63 @@
   +      $embeddedPara = $this->entityTypeManager
   +        ->getStorage('embedded_paragraphs')
   +        ->loadByProperties(['uuid' => $node->getAttribute('data-paragraph-id')]);
   +      if (empty($embeddedPara)) {
   +        return [];
   +      }
   +      /** @var \Drupal\paragraphs_entity_embed\Entity\EmbeddedParagraphs $embeddedPara */
   +      $embeddedPara = reset($embeddedPara);
   +      $para = $embeddedPara->getParagraph();
   +      if (empty($para)) {
   +        return [];
   +      }
   

   It'd be great if we can do this without loading entities. Perhaps we can use an aggregate entity query here to get the embedded paragraph ID. For an example aggregate query see \Drupal\content_moderation\ModeratedNodeListBuilder::getEntityRevisionIds

3. +++ b/src/Plugin/EntityUsage/Track/ParagraphsEntityEmbed.php
   @@ -0,0 +1,63 @@
   +      // Add tracking for internal paragraph target being used by the embedded paragraph.
   +      $this->usageService->registerUsage($para->id(), 'paragraph', $embeddedPara->getId(), 'embedded_paragraphs', $embeddedPara->language()->getId(), $embeddedPara->getRevisionId(), $this->pluginId, 'paragraph_target_id');
   

   Here we need to check if embedded_paragraphs are in the source types I think

Doesn't backport to 11.1.x cleanly and has a CR so leaving in 11.x and this will be released in 11.2

Committed d928672 and pushed to 11.x. Thanks!

This change feels correct but I think we need a follow-up to somehow make query protected in a future version of Drupal as nothing should be calling it outside the class.

I think we can change it to abstract protected function query(); and then somehow detect classes where the implementation is public. But this is all follow-up as far as I can see.

I rebased the branch on 11.x to see if we can get a green test run.

@holo96 yes.

I like performance too. But looking but any yaml parsing shouldn't really be a performance consideration. We should not be reading yaml as part of the live running of the site and we should minimise it on rebuilds to with file caching.

Committed and pushed c92120799c4 to 11.x and b436c8c9c47 to 11.1.x. Thanks!

Backported to 11.1.x as it is pure addition, recipes is still experimental and helps unblock features for Drupal CMS.

Created the follow-up - 📌 Add XSS testing for RSS descriptions Active

alexpott → created an issue.

The ability to only track some base fields mentioned #4 will be covered by #3326567: Track some base fields →

We have test coverage. This is now ready for review.

@catch I think we should do that in a follow-up.

FYI: this issue is being reverted in 🐛 RssResponseCdata filtering out common html tags Active

alexpott → changed the visibility of the branch 3497758-SubscriberFiltering to hidden.

I've added two MRs to this issue to do the revert - for 11.x and 10.x - we need a post update function because the container needs rebuilding due to the event registration.

I agree with #46 I think we should revert the cdata change. I think we already were complying to the spec as per the escaping being done by

  // The description is the only place where we should find HTML.
  // @see https://validator.w3.org/feed/docs/rss2.html#hrelementsOfLtitemgt
  // If we have a render array, render it here and pass the result to the
  // template, letting Twig autoescape it.
  if (isset($item->description) && is_array($item->description)) {
    $variables['description'] = (string) \Drupal::service('renderer')->render($item->description);
  }

The comment in #3433-60: Use CDATA in XML RSS Feeds → is not a justification for the change because we escape html in the description field. We need to test that XSS injections are filtered or double escaped.

So....

Completely refactor how we find an entity from a URL into something event based and split all of the related code out of \Drupal\entity_usage\EntityUsageTrackBase. Which is nice because that class is becoming an monster. The resulting change makes it easy to extend how we find an entity that corresponds to a URL in different cases as we now trigger an event which has the necessary info pre filled out for performance reasons and then allows us to add the necessary subscribers for each module. This means we'll do less if the file module is not installed for example.

Also the new url to entity service is needed by the entity_usage_updater module. Currently that module creates a fake tracking plugin to get this functionality :D

Need to add test coverage for the redirect code.

Given we have the issue to fix the layout shift in progress already let's go ahead here.

Committed 5413e9f and pushed to 11.x. Thanks!

I really like the idea of making this the responsibility of the thing constructing the output and not a blanket response subscriber where the security concerns are hard to impossible to consider all possibilities. Setting back to needs work to see if we can implement #43.

I think we'll also need to adjust views-view-rss.html.twig. Oh we don't need to do that because of

  // The RSS 2.0 "spec" doesn't indicate HTML can be used in the description.
  // We strip all HTML tags, but need to prevent double encoding from properly
  // escaped source data (such as &amp becoming &).
  $variables['description'] = Html::decodeEntities(strip_tags($style->getDescription()));

in \template_preprocess_views_view_rss()

Hmmm.... actually reading template_preprocess_views_view_rss()

  // The description is the only place where we should find HTML.
  // @see https://validator.w3.org/feed/docs/rss2.html#hrelementsOfLtitemgt
  // If we have a render array, render it here and pass the result to the
  // template, letting Twig autoescape it.
  if (isset($item->description) && is_array($item->description)) {
    $variables['description'] = (string) \Drupal::service('renderer')->render($item->description);
  }

I think the whole cdata approach we did can be removed. It's just not necessary for views in core. Anything in contrib that needed that should implement what views has.

I wonder if we should be concerned if there is another way of meeting the condition stripos($event->getResponse()->headers->get('Content-Type', ''), 'application/rss+xml') === TRUE and not having already filtered HTML here. Probably not from views… but what about contrib?

I think adding the CDATA sections in \Drupal\Core\EventSubscriber\RssResponseCdata means we become responsible for all ways that Drupal generates rss not just from Views and that makes this very tricky as we have to consider the security of custom and contrib code.

I think we can do something here. Redirects are not in the routing system so need it’s own thing see \Drupal\redirect\EventSubscriber\RedirectRequestSubscriber. I think we can add something to \Drupal\entity_usage\EntityUsageTrackBase::findEntityIdByUrlString() to allow modules to determine an entity for a URL and then provide an implementation for redirects in the entity_usage module.

Additionally at the moment we determine whether we look in an entity type's basefields based on a single boolean value - entity_usage.settings::track_enabled_base_fields - but we always want to track usages for redirect basefields so I think we should make basefield tracking per entity type.

alexpott → made their first commit to this issue’s fork.

alexpott → created an issue.

This was resolved in 🐛 Make link remover page work well with content moderation Active

I think this is ready now. We log any exceptions thrown by track and have test coverage that the exceptions are logged.

pameeela → credited alexpott → .

Could do with adding an automated test here.

Added logging...

I pushed an MR that starts on #5 - just need to add logging service in a BC compat way...

Or perhaps even better would be to put the try catch inside field loop in \Drupal\entity_usage\EntityUsageTrackBase::trackOnEntityUpdate() and \Drupal\entity_usage\EntityUsageTrackBase::trackOnEntityCreation()... it's tricky - that'd be more robust but potentially more likely to catch something you would want to log - so if we do that we should log as well.

Nice find... the problem here is that \Drupal\entity_usage\Plugin\EntityUsage\Track\Link::getTargetEntities should not throw the exception. We should put the. try catch in there. Also I don't think we should be logging here either. I think we should just return an empty array and move on. That means other fields with entities will still be tracked for this entity.

alexpott → created an issue.

I think with \Drupal\Tests\entity_usage\FunctionalJavascript\EntityUsageLayoutBuilderEntityBrowserBlockTest and \Drupal\Tests\entity_usage\Functional\EntityUsageLayoutBuilderTest we have test coverage of my changes.

Note that I've tested the code and there looks to be test coverage too but the massive site I'm working on does not use this plugin so I've not done a full performance test.

Yep exactly something like that... pushed a change to fix the test and to test the update.

Note that I've tested the code and there looks to be test coverage too but the massive site I'm working on does not use this plugin so I've not done a full performance test.

I've removed the exception throwing because that just should not happen here - if it did it would prevent you saving an entity for something the user cannot control or fix.

quietone → credited alexpott → .

alexpott → created an issue.

alexpott → created an issue.

This is working perfectly.

alexpott → created an issue.

>  [notice] Truncated the entity usage table
>  [notice] Created the entity usage bulk table
>  [notice] Updating entity usage for block_content: 8 of 8
>  [notice] Updating entity usage for node: 200 of 306574
>  [notice] Updating entity usage for node: 400 of 306574
>  [notice] Updating entity usage for node: 600 of 306574
>  [notice] Updating entity usage for node: 800 of 306574
>  [notice] Updating entity usage for node: 1000 of 306574
>  [notice] Updating entity usage for node: 1200 of 306574
>  [notice] Updating entity usage for node: 1400 of 306574
>  [notice] Updating entity usage for node: 1600 of 306574
>  [notice] Updating entity usage for node: 1800 of 306574
>  [notice] Updating entity usage for node: 2000 of 306574
>  [notice] Updating entity usage for node: 2200 of 306574
>  [notice] Updating entity usage for node: 2400 of 306574

Is what I'm seeing when on the latest dev version of entity usage.

The numbers only should change if you create or update content while the batch is in progress.

@marcoscano thanks for the review - replied to the commment.

This is a little easier once 📌 Remove the queue option from the rebuild entity usage command Active lands as that removes a configuration option for the defunct queue approach.

@marcoscano I created the follow-up - see 📌 Make batch performance constants configurable Active

alexpott → created an issue.

I selected defaults that worked in an environment with millions of revisions and only 512 MB available to PHP and ran drush to rebuild the table. I agree that these could be configurable - it'd be nice if it was config and drush because then if you rebuild via the UI you can tweak stuff and you can also tweak on the command level. Feel free to open an issue about that - I'll get round to it but not sure when as it won't be a priority :)

Here's a patch

alexpott → created an issue.

Ah we need return typehints...

Need the patch for a D11 site.

alexpott → made their first commit to this issue’s fork.

alexpott → made their first commit to this issue’s fork.