#Needs manual testing

The change/bugfix cannot be fully demonstrated by automated testing, and thus requires manual testing in a variety of environments.
โšก๏ธ Live updates comments, jobs, and issues, tagged with #Needs manual testing will update issues and activities on this page.

Issues

The last 100 updated issues.

Activities

The last 7 days of comments and CI jobs.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States m.stenta

    public function addSavepoint($savepoint_name = 'mimic_implicit_commit')

    Very naively (having just jumped into this issue for the first time myself), could we leverage the $savepoint_name method argument as a solution instead?

    It sounds like the issue is primarily a conflict in the mimic_implicit_commit name itself. Presumably changing $savepoint_name = 'mimic_implicit_commit' to $savepoint_name = 'some_other_name' would also solve this problem? (I'm not suggesting that... just posing the thought.)

    I don't know all of the code that calls addSavepoint(), but would it be possible to override that in the downstream modules that are affected by this issue, such that the default argument can be changes to avoid the conflict?

    Just throwing out alternative ideas here to stimulate the conversation a bit, while I wrap my head around this issue myself. Apologies if these have already been explored. :-)

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States m.stenta

    Can someone please update the issue summary to include an explanation of what the proposed change is to the logic, and why it is necessary? There are plenty of comments in this thread that say "the patch works!" but very little about what the broader implications of the change might be. Reading the code diff doesn't provide any of this, and it raises further questions.

    Changing if ($this->inTransaction()) { to if ($this->inTransaction() && !$this->transactionManager()->has($savepoint_name)) means that the addSavepoint() logic will NOT run in certain scenarios anymore. Is this OK? Has thought been given to how this might behave in other contexts? If so, I don't see any of that documented here (unless I missed it?) and if this causes issues later, future developers will have a hard time looking back at this issue and understanding why the decision was made.

    I'm not critiquing the solution itself - it might be the right answer! But I'd like to see more of an explanation so that it's easier to understand at a glance.

    Setting back to "Needs work" for the issue summary update, per @smustgrave's comment #62 above.

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia mstrelan

    Another 3.5 years on from the last comment. The patches we have here are optionally adding a class and providing styles in classy. We no longer have classy, and the remaining themes have a mix of including the icon and not including it.

    • claro - icon
    • olivero - no icon
    • starterkit_theme - icon
    • umami - icon

    Not sure what's the next step, but it doesn't seem to be related to system.module so I'm changing the component to CSS.

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany donquixote

    The question is which cache tags should be invalidated here.
    I suspect we would have to invent a new cache tag that does not exist yet.

    (here is what I remember, please correct me)

    There is one cache tag for the published / active version of an entity.
    This gets invalidated when that published / active version is updated.
    There are good reasons why we would not want to invalidate this when other revisions are deleted or updated, because we want the published display to be served from cache as often as possible.

    Instead, we could introduce a cache tag for "revision list for entity ", which gets invalidated when any revision of that entity is deleted, created or updated, or there is a workflow change.

    We would have to properly test this.
    And we need to determine if we really want to fill the cache with rendered entity revisions.
    It could be really nasty on storage, for possibly little benefit.

  • If so, which cache tags make sure that this cache is invalidated when revisions other than the published revision are added, modified or deleted?

    One thing I noticed from MR 12158 for ๐Ÿ› NodeRevisionsAllTest::testRevisions() fails in 11.1.x Active is this:

        // Delete older revisions.
        foreach ($older_revision_ids as $older_revision_id) {
          $node_storage->deleteRevision($older_revision_id);
        }
    
        // At first, the old list is still in the cache.
        // @todo This is bad, the list should be updated.
        $this->drupalGet('node/' . $node->id() . '/revisions');
        $this->assertSession()->responseContains('page=1');
        $this->assertSession()->pageTextContains(end($logs));
    

    Drupal\Core\Entity\ContentEntityStorageBase::deleteRevision() does not invalidate cache tags, afaict. Looks like all the cache tag invalidation for entity CRUD operations occurs in the entity class and not the storage class, and delete() is never called on the loaded revision in deleteRevision(). I'm not sure whether this is a bug or as intended.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Thank you for creating this issue to improve Drupal.

    We are working to decide if this task is still relevant to a currently supported version of Drupal. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or is no longer relevant. Your thoughts on this will allow a decision to be made.

    Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

    Thanks!

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    I don't think we differentiate between default and non-default revisions when invalidating cache tags so it should actually be invalidated correctly. Didn't test this but couldn't find any differentiation.

    Whether that's in itself a caching bug (or lack of feature) is a different question though. We could add an explicit test that it gets invalidated, and then if we stop invalidating tags for non-default revisions it would cover that case.

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia acbramley

    Can and should we cache a revision list?

    I think in a real world scenario we'd almost never hit the bug that this test is hitting. To reproduce it via a browser I had to:

    1. Disable the admin theme setting for node.settings
    2. Disable the breadcrumb and primary tab blocks (these add the node cache tag)

    Without doing this the node cache tag gets added to the page and when editing a node via the UI to create a new revision, the revisions page is purged.

    I think it's perfectly reasonable to have a cacheable revisions page, especially since the reason why it isn't is because of a bugged call to addCacheableDependency (which I thought we deprecated in #3232018: Trigger a deprecation when using \Drupal\Core\Cache\RefinableCacheableDependencyTrait::addCacheableDependency with a non CacheableDependencyInterface object โ†’ but that didn't cover the renderer...)

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany donquixote

    This default setting was changed in #3347015: Consider using the administration theme when editing or creating content by default, does that just need to be committed to 11.1.x?

    I would say this is a significant functional change that we would not want to have in 11.1.x.
    If all we care is to make a test work, we can change that setting in the test itself.
    Or rather, we change that setting in the test to use the front-end theme instead of the back-end theme, if we are interested in the cache behavior.

    Then, the more important question is:
    Can and should we cache a revision list?
    If so, which cache tags make sure that this cache is invalidated when revisions other than the published revision are added, modified or deleted? I don't think such a cache tag exists currently.
    Also, what is the use case where caching the revision list actually makes sense? That is, where the revision list is visited significantly more often than it is changed? (I suppose working with revisions in workflow)

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia acbramley

    Here's the crucial difference:

    11.x has node.settings:use_admin_theme defaulting to true https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/node/...

    11.1.x has it defaulting to false https://git.drupalcode.org/project/drupal/-/blob/11.1.x/core/modules/nod...

    DPC has a response policy to blanket deny on admin pages via Drupal\dynamic_page_cache\PageCache\ResponsePolicy\DenyAdminRoutes

    This default setting was changed in ๐Ÿ“Œ Consider using the administration theme when editing or creating content by default Active , does that just need to be committed to 11.1.x?

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia acbramley

    It looks like the difference is to do with some other unrelated cache metadata making this cacheable on 11.1.x when it's not on 11.x (in the context of NodeRevisionsAllTest)

    11.x cache headers:

      'Cache-Control' => 'must-revalidate, no-cache, private',
      'X-Drupal-Dynamic-Cache' => 'UNCACHEABLE (response policy)',
      'X-Drupal-Cache-Tags' => 'http_response rendered',
      'X-Drupal-Cache-Contexts' => 'languages:language_interface theme url.query_args user.permissions user.roles:authenticated',
      'X-Drupal-Cache-Max-Age' => '-1 (Permanent)',
    

    11.1.x cache headers:

      'Cache-Control' => 'must-revalidate, no-cache, private',
      'X-Drupal-Dynamic-Cache' => 'HIT',
      'X-Drupal-Cache-Tags' => 'http_response rendered',
      'X-Drupal-Cache-Contexts' => 'languages:language_interface theme url.query_args.pagers:0 url.query_args:_wrapper_format user.permissions user.roles:authenticated',
      'X-Drupal-Cache-Max-Age' => '-1 (Permanent)',
    
  • @acbramley opened merge request.
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    The fix for the regression in 11.1.x isn't straightforward and it's been in 11.1.x for over a month, it's also not clear why this is passing on 11.x, given both sides of that are confusing, I'm going to go ahead and revert here. There is the start of a fix in ๐Ÿ› NodeRevisionsAllTest::testRevisions() fails in 11.1.x Active .

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia kim.pepper ๐Ÿ„โ€โ™‚๏ธ๐Ÿ‡ฆ๐Ÿ‡บSydney, Australia

    Still a few unsresolved comments on the MR

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany donquixote

    If a page was not cached before, it is not enough to just fix the reasons why it was not cached.
    We also need to check that all the cache metadata added to the page is complete!
    I suspect we have missing cache tags..

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany donquixote

    We have a failing test in 11.1.x:
    Drupal\Tests\node\Functional\NodeRevisionsAllTest::testRevisions()

    git bisect reveals the failure was introduced here.

    For some reason, it was not failing in the MR when it was merged.

Production build 0.71.5 2024