🇺🇦Ukraine @osab

Thank you @ultimike, you're right, I think. I've updated N-naming to "N- no specific cache properties" here https://www.drupal.org/project/cache_review/issues/3446118 📌 Add basic test and setup CI Needs work in one of commits.

Hi!
I've updated naming in https://www.drupal.org/project/cache_review/issues/3446118 📌 Add basic test and setup CI Needs work . Only couple blocks and all others are sections there.

Thanks a lot, @ultimike!
I will take this into account and think afterwards how to improve the analysis and description of such cases.

fixing https://www.drupal.org/project/cache_review/issues/3446118 📌 Add basic test and setup CI Needs work is in progress...

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

Thank you, @andypost!
Here is fix for 1.1.x
https://git.drupalcode.org/project/cache_review/-/commit/94dfc4eef0a9085...

I also updated 1.0.x
https://git.drupalcode.org/project/cache_review/-/commit/e09bc2cc9d0c58b...

hi!
Yes, it will be greate to have more clarity in Drupal Cache system ))

-

I am not entirely clear on the difference between "C" and "N" in the output, however.

The idea was to have
C - for cached items,
L - for lazy built items and
N - for others items without specific cache properties that take part in bubling results of caching page.

Maybe it will be better to have description: C - item cached, L - item wrapped by Lazy builder, N - non-cached item without lazy.

IPC - just for information, I think. It's kind to show that it is anonymouse and that's why IPC =HIT.

-

Also - according to the Drupal core BlockViewBuilder class, pretty much all blocks other than the branding and title blocks use lazy_builder, no? If so, why don't I see an "L" when using Cache Review on menu blocks (as well as a few others)

So, yes, good question ). In general, I see that such block (for instance "block-olivero-site-branding") is cached despite '#lazy_builder'. The question is why and what to show for users in this case.. Let me a bit more time to sort out with this case. Thank you for helping on improvement!

Hello Michael! Thank you for the question. I will try to answer based on my understanding. Please do not consider my answer to be absolute truth )).

So yes, I see such behavior for admin user also. Here the "keys":["demo_child"] plays role.
- On the page we have setting "contexts":["url.query_args"] (block 04). That means to invalidate cache if URL query arguments changed, what happened after adding ?test=1.
- We have "max-age":20 also. So, 20 sec the page is unchangable (block 02).
- But we also have "max-age":40,"keys":["demo_child"] (block 03). And this block from 21 to av. 40 second will stay freezed (cached) because of "keys". The "keys" is like specific mark means "do not touch this part during invalidation because of some "heavy" calculation here or smth". And then, after 40 sec the "max-age" comes into force and block 03 updates.

To make sure, that's all works in this way, try to change parameters. For example if you remove "max-age":40 for block 03, after 20 sec all others will get new times, but block 03 become cached.
At the same time, if remove "keys":["demo_child"], the block 03 will be changed after 20 sec along with others.

More info I saw here https://bluetext.com/blog/drupal-8-render-caching/,
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Render%21....

As for "block" naming, yes you're right. That is not good choice. Initially, I've added plugin blocks for lazy-builder and then called other parts as "blocks" also. I don't know, maybe "sections" wil be better? What would you advise?

hi @apaderno! Could you help once more and review the module after my changes? Thank you in advance!

updated.
- Set >10.1 version and variation_cache_factory only.
Please, review whenever you will have time. Thanks a lot!

Thank you, @apaderno! So, if I understood correctly, the better way is to remove version_compare in code, like
version_compare(\Drupal::VERSION, '10.2', '<'), add 'variation_cache_factory' service only and restrict module to core version 10.1 and higher?

Thank you @apaderno and @vishal.kadam for help!
I've removed Lisence file and add phpcs fixes (looks like I have some options and resctrictions in phpcs CLI command before )) ).

As for cache_review/src/Render/RenderCacheReview.php I need your advices how it can be made better.
The point is I wanted to make my module work on Drupal 10 and Drupal 11. That's why I can't set 'cache_factory' (for Drupal 10) or 'variation_cache_factory' (Drupal 11) as arguments in servise file directly.
So, I've found the same approach in core/lib/Drupal/Core/Render/PlaceholderingRenderCache.php:

  public function __construct(RequestStack $request_stack, $cache_factory, CacheContextsManager $cache_contexts_manager, PlaceholderGeneratorInterface $placeholder_generator) {
    if ($cache_factory instanceof CacheFactoryInterface) {
      @trigger_error('Injecting ' . __CLASS__ . ' with the "cache_factory" service is deprecated in drupal:10.1.0 and is removed from drupal:11.0.0. Use "variation_cache_factory" instead. See https://www.drupal.org/node/3365546', E_USER_DEPRECATED);
      $cache_factory = \Drupal::service('variation_cache_factory');
    }
    parent::__construct($request_stack, $cache_factory, $cache_contexts_manager);
    $this->placeholderGenerator = $placeholder_generator;
  }

Maybe it will be Ok to implements ContainerFactoryPluginInterface and add this condition in create() method, but I'm not sure that is correct approach.
Thanks in advance!

osab → created an issue.

fixed in 1.1.x version

Hi! Thanks for the fixes.

Hi! Thanks for the fixes. It should be Ok in 1.1.x version. At least, Di was added in all places, I hope ).

Hi! The version 1.1.x was tested on Drupal 9, 10 with image on the node page and I didn't see the bug. Please check new version and reopen if the bug still presence. Thanks!

yes, looks like it still actual for 1.1.x. Will take a look.

I think we can fix this by moving
core.date_format.appointments_date.yml
workflows.workflow.appointments.yml
to appointments/config/install/optional.

Looks like we need to fix at least two points to start Appointments on Drupal 10:

1. Add these rows to annotation of Appointment.php and Applicant.php

 *   revision_metadata_keys = {
 *     "revision_user" = "revision_user",
 *     "revision_created" = "revision_created",
 *     "revision_log_message" = "revision_log_message",
 *   },

2. Replace

  ->setDisplayOptions('....', [
         ...
        'type' => 'hidden',
      ])

by

  ->setDisplayOptions('....', [
         ...
        'region' => 'hidden',
      ])

to fix the issue "The "hidden" plugin does not exist - Drupal 9 Compability" →

And of course, we need to implement fixes from patch "For Drupal 9 compatibility - deprecated functions" → .

Lets review others possible bugs on Drupal 10 and gather them here for future patch. ))

osab → created an issue.

+

Do we have a guide about using DDEV for site development? I think it deserves to be mentioned here.

it seems make sense to close the issue as no activity for a long time.

#12 works fine for me also, D10.1.5 php8.1. It fixed the error 'User %user not allowed to go from state %sid1 to %sid2' from workflow module, as it made migrate as anonymous.

osab → created an issue.