🇵🇱Poland @sayco

Done, I hope this time we'll manage to push it forward before anything else is merged to dev branch :)

Ok it succeeded, so it seems like only CacheGetCount was reduced

I changed the value back to 81 and let's see the pipeline, I suspect that it will fail on the other key in $expected array in the same test.

Ok I rolled back the changes and the pipeline fails, so the change was indeed needed, the CacheGetCount is reduced
├ Failed asserting that 81 is identical to 82.

Should I rollback the changes to the metrics I did and we will see if the test runs fine without the changes?

But maybe you @sayco re-ran tests lots of times prior to me?

@oily I intentionally left it in a failed state, to prove that the tests tend to fail on that test case.
It's good to see that it is all green now!

We'll have to fix this either here or a separate issue and postpone it on it.

@berdir the Cron tests should be handled in a separate issue IMO, but it shouldn't be a blocker for this ticket (they seem to be unrelated, especially if it's a known failure)

I've addressed all of the comments I guess, but the pipeline keeps failing because of the cron tests, I'm not sure what we are going to do about it - I suppose it's out of the scope of the current issue.

First of all, I think the ticket needs to be updated with the information that the fromInternalUri is now part of the Url class, not the UrlHelper.

The other thing about this (before we apply any changes in the code) is that the implementation of the function is wrongly designed. It breaks the SRP, as it’s not only responsible for creating the internal URI, but also for validation against the external URI. I think this is the wrong approach.

I know it requires a lot of changes, but the fromInternalUri should be responsible for building the internal URI from whatever is given, or it should return a null value or raise an exception. Whatever is using the function should be responsible for handling the exception or null value, checking what the given value is, and taking appropriate actions depending on the outcome. It shouldn’t be the responsibility of the fromInternalUri method to know about other dependencies and all the use cases.

We also should avoid adding any silent caches in the code. At the very least, logging with debug level should be added. Otherwise, it’s hard to guess that anything unexpected happened in the code.

I've updated all the tests, seems like the number of cache get's has changed because of the routing which is now checked as first. The cron UI test was wrong from the very beginning because the intention was to create the admin user to validate some text in the admin panel, but in fact the regular user was created with just a single permission and it's !== admin user.

I've opened MR with all the needed changes, but I still work on tests. Some unexpected issues takes place and I'm not sure how it's related to our changes, I'm still investigating it.
Regarding the deprications I used the proposed trait to handle the deprecated service.
BTW I confirmed that checking the permissions is slower then checking the routes, both numbers are quite low, so it's not something that anyone can notice, but it is always good to reduce the unnecessary operations. The logical implication is the same, if the route is not the admin route, then there is no reason to check any permissions.

I'm going to pick up this one. It seems that we can safely remove the check. The negotiator and the storage is provided by one and the same module which is part of the core and can't be installed.
I would also add some improvement to check the route first - there is no way to trigger any permission checks if the rote is not an admin route (the route object is available from request, so it's no heavy resources operation).
For the entity type manager, we have to keep it as it is (the constructor parameter) but the deprecation notices must be added, I will try to handle it in the scope of the issue. I'm not sure what should be the target version after this will be completely removed.

Merged, it will be part of the next minor release.

Thanks for contributing! It is now merged to develop branch and be part of the next upcoming release

Hi! You are right, I've added a gitlab pipline configuration yesterday and it failed for the same reason - the TYPO :(

I fixed the issue and I'm about to add new release soon.

I just pushed the change by removing the hook install to a dev branch.
This will, of course, won't help your issue (only the workaround I mentioned will help you),
but this will prevent future installations from falling into the same issue.

I consider the issue fixed, and for future reference I'll paste again the workaround:

One very simple workaround for this is to add a paragraph field to any paragraph type and then remove it.

Hi, thanks for reporting.
It seems to be indeed the issue.
One very simple workaround for this is to add a paragraph field to any paragraph type and then remove it.
It will trigger the internal Drupal logic to remove the field storage if the field is no longer used in any entity (see video attached).

I don't think that what was proposed in a patch makes sense, to be honest.
The code is correct, but I don't think we need this complex solution for a simple issue.
The original issue is that on hook_install the field storage is created, but it should be created "on the fly" when creating the field.
So to fix the issue permanently, the hook_install has just to be removed.

The change looks fine, I've merged it to the dev branch.
Setting this to done ;-)

The MR was a few commits behind the dev branch. I applied the changes manually and committed them to the dev branch. Thanks for contributing!

Hi, yes we are planning to make the module compatible to D10 but hoping that community will help a bit (especially with testing the patches proposed by the BOT)

I will close this ticket, please track the https://www.drupal.org/project/autotitle/issues/3407301 📌 Automated Drupal 10 compatibility fixes Needs review
for the updates regarding the D10 updates

The patch is merged, but I had to adjust the patch a bit to be compatible with the latest dev version.
Thanks for contributing!

Merged to dev version, the fix will be included in the next release. Thanks for contributing

Hi, thanks but this would be to use. I forgot to mention, that we should also do some cleanup in the code, such as

• define typing - for the properties, the arguments, the return types
• add strict_types everywhere
• remove the deprecated code
• use the modern PHP syntax where applicable

I'll update the issue description. It would be great if you could introduce these changes and test them.

sayco → created an issue.

Thanks for adding the patch. I just created a new stable release which includes this change, so you can upgrade your project version.

sayco → created an issue.

I did some testing and it seems like the issue is solved. If anyone falls into trouble again, we have a quick workaround but a better solution must be found.

Thanks @james.williams! The changes were fine, but I realized that I need to add some improvements to the readme anyway so I committed it along with your changes!
This will stay in dev for some time, but it will be of course part of the next upcoming release.
Thanks once again for contributing!

I'm sorry I forgot to add "in a README.md" file, so this is where it needs to be described.

Hi @james.williams thank you for contributing!

I've reviewed and tested the patch and it seems to be working fine. The code quality is very good - thank you for that.
Before I apply the patch, could you provide the updated patch by adding the description in the configuration section of how this Apply preview feature works (what is this for)?

So about the topics you've mentioned here.
1. The configuration - this is something I was thinking of at the beginning of the project, so I assumed that the widget would be a better place because you might use a different configuration depending of the form mode. For example, you can have view modes: A, B, C, D, and Form modes X and Y. The X will allow you to select A and B, and you have the same component in the form mode Y which will allow you to select only C and D. I hope it makes sense. The new thing is this form mode bind, this will indeed require from developer to set things up correctly, otherwise, you won't be able to go back to the previously selected mode. Nevertheless having this on the widget level allows us to do such things.
2. The preview mode - well this was reported by the community and AFAIK this is the view mode used for other paragraphs' ecosystem modules to preview the paragraph (not sure which - most probably the new widget for the paragraph), so I believe this one requires special treatment, but as you proposed it is wise to have the ability to turn this off. I think it is fine to that that on a widget level.

I had to apply yet another fix (and remove the previous change) because I broke more stuff than I fixed :D
It seems now that the ajax callback is working a expected

I've merged a change to the dev branch. It seems like it will fix the issue, but it would be great if someone could also test it anyway.

greggles → credited sayco → .

Closing as it seems like this is only related to the Marketing API

sayco → created an issue.

Any updates on this?
Opposite to the comment above, I think entity storage is not the only case, there are multiple cases such as taking objects from factories - the config, queue, plugins, etc.
If there is a chance that it is easy to integrate (compatible with Drupal core container), then it is worth giving a try, especially if good practice/SOLID is at stake.

Thanks, I've already made a release of the 3.1.2 version so this will stay for some time in dev, but before being merged please review and fix the comments, thank you!

I wasn't able to reproduce the issue unfortunately and don't know what was going on.
It could be reopened or a new issue created once the issue comes back for someone else.
We are now focusing on 3.x version only so I hope it won't happen ever again.

Merged, thanks for sorting it out so fast :-)!

Thank you for reporting! I've added a small change request to NULL instead of FALSE.

Hi, this kind of error usually appears when you do not clear the cache after installing the module. Have you cleared the cache?

It seems like the issue is related to the https://www.drupal.org/project/drupal/issues/2504115 →

I've figure out some sort of solution proposed here for a views module
https://www.drupal.org/project/drupal/issues/2504115 →

which will allow us to access the original views_path that is sent over with the main request.
I've add my own implementation to the hook_preprocess_views_view, but for the commerce_cart module it could be more or less as follows:

function commerce_cart_preprocess_views_view(&$variables) {
  $view = $variables['view'];
  if (strpos($view->storage->get('tag'), 'commerce_cart_form') !== FALSE) {
    $variables['rows']['footer'] = $variables['footer'];
    $variables['footer'] = '';

    $request = \Drupal::requestStack()->getMainRequest();
    $is_ajax_reload = $request->isXmlHttpRequest() && $request->request->get('_drupal_ajax') === '1';
    if (FALSE === $is_ajax_reload) {
      return;
    }
    $variables['rows']['#action'] = $request->request->get('view_path');
  }
}

Seems to be working as expected now.

My issue is partially related to the topic, but still, I'm here because of that piece of code

      // Remove all of this stuff from the query of the request so it doesn't
      // end up in pagers and tablesort URLs.
      // @todo Remove this parsing once these are removed from the request in
      //   https://www.drupal.org/node/2504709.
      foreach ([
        'view_name',
        'view_display_id',
        'view_args',
        'view_path',
        'view_dom_id',
        'pager_element',
        'view_base_path',
        AjaxResponseSubscriber::AJAX_REQUEST_PARAMETER,
        FormBuilderInterface::AJAX_FORM_REQUEST,
        MainContentViewSubscriber::WRAPPER_FORMAT,
      ] as $key) {
        $request->query->remove($key);
        $request->request->remove($key);
      }

I really need access to the original data from the request (as for many of you my $form['#action'] is incorrect).

My proposal to sort this out is to clone the request object and push it to the request stack.
Thanks to that we can still get the main/master request and the current one.

In my case, I just simply wrote the Route Subscriber and alter the `views.ajax`
route with a custom controller.
In the controller I simply do the following:

  /**
   * {@inheritdoc}
   */
  public function ajaxView(Request $request) {
    $request = clone($request);
    $this->requestStack->push($request);
    
    return parent::ajaxView($request);
  }

The view is working without any issues (at least seem to be working fine), and I'm able to now do whatever I want with the data from the main request by

 $request = $this->requestStack->getMainRequest();

I hope this workaround will somehow help you with finding the right solution.

Unfortunately, seems like nothing was committed to the 6.4.x nor the 6.5.x, there is still the 2.6 version.
Along with the update also the patch
"Trying to access array offset on value of type null in EntityBrowserElement valueCallback": "https://www.drupal.org/files/issues/2021-02-19/3199269-array-value-null.patch"

Should be removed, as it is already included in 2.9 and will break the update.

@scotwith1t Thank you for sharing your thoughts on this issue. It's as @jacobbell84 said. The main difference between the original patch and the new approach is that the latter uses an early return statement instead of executing the code in the if statement. However, both versions should function the same way and address the underlying concern.
Based on that fact I assume that the original problem has been resolved? Is this correct? Or is it for some reason working differently for you on the dev branch?

If you feel that there's a specific problem that's different from the original issue (which in my opinion was solved), it would be best to create a new issue and provide detailed information there. This will help to ensure that the issue is properly tracked and addressed.

BTW it is worth mentioning that I have implemented a proper checking, but for the ParagraphInterface instead of the entity id. Please have look on DisplayModeMatcher https://git.drupalcode.org/project/paragraph_view_mode/-/blob/3.x/src/Matcher/DisplayModeMatcher.php#L80 for more details.

Hi @rodrigoaguilera! Thanks for such a write-up and so many kind words on this particular module.
The thing about `invoke` in Drupal core is also a bit strange to me.
The Drupal\Core\Extension\ModuleHandlerInterface the invoke "Invokes a hook in a particular module" in contrary to the `invokeAll` which "Invokes a hook in all enabled modules that implement it."

From what I can see in the core, it is used mainly for the hooks like `install`, `uninstall`, `schema`, `mail`, `help`.

I've decied that in case of the `invoke` which is called per module there is no point to dispatch the event for a specific module as well if it already has this hook implemented to avoid redundancy.
I don't think this limitation do any harm, but if there will be a need for chaning this behaviour we can of course consider to change it.

I hope my answer will somehow help you when working with the module.

If you consider my answer as complete, please mark the issue as fixed, thanks!

I was a bit shocked about this fact! :D And I am a bit worried that the community had time to standardize such a thing whereas there are tons of issues for the Drupal core ^^' Anyway, thanks! Merged to dev branch.

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

Thx, RenatoG! For the mixed type, it's as you said - the PHP 8.0+ feature, so it should stay as it is.
I also understand the point of ignoring the README.md, having code samples makes it almost impossible to keep with that limit (I tend to like the PSR-12 120 chars limit BTW).

Regarding the DI, I really wanted to push this module to the stable release.
This was the only piece of the puzzle that I couldn't figure out. I was trying to make it
the same way as the circular reference is done at the core for the theme.manager

  theme.manager:
    class: Drupal\Core\Theme\ThemeManager
    arguments: ['%app.root%', '@theme.negotiator', '@theme.initialization', '@module_handler']
    calls:
      - [setThemeRegistry, ['@theme.registry']]

strangely without any luck.
I don't have that much time recently so any help on this issue will be appreciated.

I was about to ask about that topic in the relevant README.md issue, but you seem to be much quicker! Perfect! Thanks

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

Meged to 3.x, thanks!

Thank you for reporting indeed seems like I was too strict (actually I wasn't aware that Core does not have strict typing for this argument).
I also agree that the relevant changes should be done first in Drupal core, then we could adapt it to a more strict mode.
I've merged your patch partially. I've reworked a bit of it related to the if statement - use the early return instead of wrapping the whole block with if.

Thank you for reporting, the MR was approved and merged to dev branch.

As I wrote, in order to fix the missing service you have to clear the cache, however, there was yet another bug caused by the apparent mistake. It seems like when I stashed changes locally I haven't noticed that I'd wrongly solved the conflict and mixed up the name of the methods.
Please pull the latest 3.1.1 version which should be free of this bug.

Have you cleared the cache? This is the only thing required for Drupal to recognize new services.
Let me know if this will help.

There were some changes in documentation, I couldn't merge the MR.
I've simply updated the readme myself, but thanks anyway for trying to improve the module!

@Jay Jangid, thank you for taking care of the documentation! Please review the comments above that were added to the MR, as these needs to be fixed.

Finally, I've managed to make it work! The latest changes have been pushed to the dev branch. Hope some of the sites will start using it and we will receive a valuable feedback

Added to the project description! Thanks!

I have some progress on this issue. I hope I'll be able to push something to dev within a few weeks.