🇺🇦Ukraine @andriy khomych

IMHO, we should presave extensions keys.
It is useful information and can be used in the AlterableComposableSchema.
Opened issue https://www.drupal.org/project/graphql/issues/3515245 🐛 Sort extensions by priority and presave extension key. Active

Attached static drupal patch.

andriy khomych → created an issue.

Hey Pieter Frenssen.
I think we have something similar. Did you check https://git.drupalcode.org/project/graphql/-/commit/4f92c531593465898b20... ?
Based on the plugin's weight the full graphql file content has to be replaced.
IMHO, if we can override the AST tree content part it could be much more flexible.

Thanks Klausi.
Looks good to me and solves this issue.
However, we have failed pipelines (one is PHPCS more issues, and another is PHPStan more issues).
I think they can be done in follow-ups.
It has been moved to RTBC.

Thanks Klausi.
Looks good to me.
It has been moved to RTBC.

Hey Klausi!
I checked it and it looks good to me.
Moved to RTBC.
Thank you!

Thanks, Klausi!
I checked this MR, looks fine to me. Moved to RTBC.

Thanks, looks fine to me.
Moved to RTBC.

I checked the above patch and MR.
In my opinion, it is fine.
Moved to RTBC.

I might help with some questions.

>It seems like this contains #3479204: Fix views data field id for boolean in it
I can answer this. It has it.

> Out of curiosity, what happens if you try to use a new property in a condition? Does that work?
I can answer this. Yup, it works. That's why I like this MR :)

Thanks Klaus.
I checked it and moved to RTBC.

Attached patch.

andriy khomych → created an issue.

Thanks @klausi!
I cannot approve this PR, but it looks fine to me (as well it worked fine on the manual test).
Moved to RTBC.

Attached patch from MR.

+1 one for this patch.
I have a custom entity type, for which I was provided some custom tokens, but there is no way to have standard tokens work (like id, label, etc.).
This patch solves the issue.

I faced the same issue and the above patch works fine.
I guess, such cases should be covered with tests.

Attached patch for 4.2x

Attached patch.

andriy khomych → created an issue.

Thanks, Klausi! Well done, it looks good to me.

Unfortunately, I cannot add a review note to my MR, overall, it looks good.
One more moment about cache:
$context->addCacheableDependency($item->access);
I think we might add a menu object as well. I'm unsure when we have a menu link removed or added so that correct information is displayed.

@andriy khomych I added cache context and 2 test cases.

Do you need the access result neutral check? We should not allow neutral access results, right? Only menu links where the user has access should be returned.

Yup, Drupa core allows only allowed links.
MenuLinkTree::buildItems

      // Only render accessible links.
      if ($data->access instanceof AccessResultInterface && !$data->access->isAllowed()) {
        continue;
      }

Prepared MR and attached patch.

andriy khomych → created an issue.

It is still a valid issue for multilingual sites.
However, the above patch is breaking import, and roles are not receiving permissions.
Here is a new patch fix.

Hi, you must use any method from my comment #2.
It is not this module issue.

My slack nickname - @andriy.khomych
Slack channel - drupal.slack.com

Hey ananda, well, I don't think it should be converted, this module covers specific field configurations in the importer.
It sounds more like a misconfiguration, or missed plugin, I think you should have in the export correctly decoded values.
Example of working XML source:

<description>
<![CDATA[ <p>

However, if you need to support your case, I think we can manage the custom feeds plugin (your project plugin), you can try more plugins from https://www.drupal.org/project/feeds_ex → or modify data using feeds events:

 your_module.importer.event_subscriber:
    class: Drupal\your_module\EventSubscriber\ImporterEventSubscriber
    tags:
      - { name: event_subscriber }

  /**
   * Event triggered during the parsing of the feed.
   *
   * This is called after actual parsing is done. See the priority in
   * getSubscribedEvents().
   *
   * @param \Drupal\feeds\Event\ParseEvent $event
   *   The parse feed event.
   */
  public function onParse(ParseEvent $event): void {
foreach ($event->getParserResult() as $item) {
      /** @var \Drupal\feeds\Feeds\Item\DynamicItem $item */
      $guids[] = $item->get('xpath_feeds_item_guid');

      // Decode all HTML entities in all the text to avoid problems with data
      // that is ran through check_plain() when being displayed.
      $item_array = $item->toArray();
      foreach ($item_array as &$result) {
        if (is_array($result)) {
          foreach ($result as &$value) {
            $value = html_entity_decode($value, ENT_QUOTES, 'UTF-8');
            // Remove Unicode control characters and the Unicode Private Use
            // Area which is undefined.
            $value = preg_replace('/[\x{007F}-\x{009F}\x{E000}-\x{F8FF}]/u', '', $value);
          }
        }
        elseif (is_string($result)) {
          $result = html_entity_decode($result, ENT_QUOTES, 'UTF-8');
          // Remove Unicode control characters and the Unicode Private Use Area
          // which is undefined.
          $result = preg_replace('/[\x{007F}-\x{009F}\x{E000}-\x{F8FF}]/u', '', $result);
        }
      }
      $item->fromArray($item_array);
    }

Also, this module https://www.drupal.org/project/feeds_tamper → provides a way to modify data and you can try to decode it first.

Could you ping me in Slack in Drupal?

I can confirm that I have the same issue.
In my case it was decoupled Drupal site and 2 ECA was triggered:
- Entity insert
- Entity update
- And related queue process

Updating to 2.0.1 fixed an issue.
It was hard to find it on the site and even debug it, in my case it ended with the error:
Serialization of 'Symfony\Component\HttpFoundation\File\UploadedFile' is not allowed.

I can confirm that it works fine, after updating to Drupal 10.3.1 and enabling config_inspector module I got the above error.
The provided PR fixes it.

Can confirm that it works fine, attached patch from MR 106.

I can confirm this issue, it even happens when you enable scheduling.
The #5 patch works, thanks.
I am moving it to RTBC.

It seems connected to Configuration Views (webform) skipping 'sequence' config items 🐛 Configuration Views (webform) skipping 'sequence' config items Active

Short feedback on this issue, I've tested both MR 7 and MR 9 and both are not supporting nested settings.
Example schema file:

transaction.type.*.third_party.credits:
  type: mapping
  label: 'Credits settings'
  mapping:
    parent_credit_type_id:
      type: string
      label: 'Parent credit type ID'
    node_types:
      label: 'Node types'
      type: sequence
      sequence:
        label: 'Node type'
        type: string

So, if we have a setting as an array of values, it won't work.

Fixed

Andriy Khomych → created an issue.

Well, no opinions, so we can mark it as fixed.

It would be great to have another opinion here based on https://www.drupal.org/project/taxonomy_manager/issues/3445744#comment-1... 🐛 Missing modules after updating from 2.0.10 to 2.0.11 RTBC

Andriy Khomych → changed the visibility of the branch 3447536-stop-submitting-form to active.

Andriy Khomych → changed the visibility of the branch 3447536-stop-submitting-form to hidden.

Andriy Khomych → created an issue.

Added to dev version README updates.

> As far as I can tell, one of @arx-e's suggestions in #8 should be done. If you plan not to do either of those, then there should at least be clear instructions in the module description and Readme file that term_merge must be installed and enabled separately.

I don't know if it is the right approach, generally, I don't like the idea of getting all dependencies in the composer if they are not used.
So, I'll update README.md and let's community decide this in the future.
BTW, I cannot update the module README, so we can move it to RTBC.

Andriy Khomych → changed the visibility of the branch 3445744-missing-modules-after to active.

Andriy Khomych → changed the visibility of the branch 3445744-missing-modules-after to hidden.

> p.s. Thank you for your efforts in improving Taxonomy Manager.
My pleasure.

> Personally, I prefer not to have to manually install a separate module to achieve the merge functionality. So I would tend towards the option of adding the dependencies of the submodule in the main module's composer file. But will that cause problems with minimum stability level because term_merge and term_reference_change are both still in beta, and taxonomy_manager is not? (I don't have the experience to know)

This is unfortunate, but it is known Drupal limitation regarding submodules, let's hope it can be fixed in the future.

> Another option might be to make a 2.0.12 that reverts to 2.0.9 (removing merge functionality and dependencies), and also make a 2.1.0-beta1 that includes merge. But care would be needed not to leave users with more config issues. I guess the consensus might be that it is now too late for that approach.

Yeah, it is too late for this, I tried to keep this option built-in, but due to the critical status of the issue and discussing it with another maintainer we agreed to use submodule.
Moving to fixed now.

Congratulations Andrii Chyrskyi!!!
You are now a co-maintainer!!!

Well, the original idea was to have a merge functionality as part of the main module.
I don't know if having it now in the main module composer dependencies is the right approach.
Maybe, would be enough to update module REAMDE.md and provide a note about requiring term merge and term_reference_change manually.

Hey La558, I was not able to replicate this issue using a fresh 2.0.11 version
Did you clear the cache after updating to 2.0.11?
Could you uninstall the module and install it from scratch 2.0.11?

Hey Nick Hope!
So, I suggest following steps from https://www.drupal.org/project/taxonomy_manager/issues/3445744#comment-1... 🐛 Missing modules after updating from 2.0.10 to 2.0.11 RTBC
In nutshell:
1. Use composer to include term_merge and term_reference_change
2. Enable those modules using drush en -y term_merge term_reference_change
3. Uninstall them drush pmu -y term_merge term_reference_change
4. Clear cache
5. Remove modules from composer

I think it should help, sounds like you have some config left-overs.

> One suggestion: When making a release, make an -rc release and wait for it to be two weeks old without any release-blocker issues arising before making a full release. Most problems like this would be caught in the release candidate stage.

Thanks, I will take it into account.

> At least the release notes should have made some notes about the changes and possible outcomes.

Yeah, I've updated the release notes.

> So, removing that dependency and publishing yet another minor, just broke some sites again, since an enabled module is no longer available, as it's removed from the requirement list. :-( Just saying, as people who fixed the problem manually, they just got hit a second time.

I see :( Well, in such a case I suggest adding them manually back and uninstalling them. Sorry for the inconvenience.

Marked as fixed.

Merged, moving to fixed.

I think we can use this fix.
Generally, it should not be an issue if you have https and domain locally.
E.g. I didn't reproduce it using ddev tool.

Hey Thomas :)
I've now faced another issue when we have multiple tracking/untracking/tracking/untracking/tracking/untracking in the same request
it can end up with a tracking entity that should be untracked and stored with tracked status (which is incorrect).
What about queueing tracking calls and executing the latest one after the actual request finishes? E.g. by using https://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/drup...

Hey Rajab, my pleasure to help.
Well, I thought about adding your patch, but my time just ended :)
Could you prepare a new MR? Cause you cannot update an old one.

Well, sorry for this, actually, it should work without term merge, anyway, could you check if it is still an issue with 2.0.11 version?

The issue should be fixed here - https://www.drupal.org/project/taxonomy_manager/issues/3444071 🐛 Version 2.0.10 breaks existing installations Needs work

Please, use 2.0.11 release.

The issue was fixed here - https://www.drupal.org/project/taxonomy_manager/issues/3444071 🐛 Version 2.0.10 breaks existing installations Needs work

Please, use 2.0.11 release.

> Uhm, the merge request already includes the new submodule: Taxonomy Manager Merge. All code related to the integration of Term Merge went into this module.

Yup, I worked fast on this issue, so in case we have some submodule issues, let's open new issues :)

Sounds like a duplicate of https://www.drupal.org/project/taxonomy_manager/issues/3220993 📌 Option to merge terms (Term Merge integration) Needs review
The original issue was fixed, moving it to close.

You should save your lock file using CVS system (git IMHO the best).
If you have an issue revert the lock file and run composer install to use proper lock deps.

BTW, https://www.drupal.org/project/taxonomy_manager/issues/3444071#comment-1... 🐛 Version 2.0.10 breaks existing installations Needs work is fixed.

A new release is here - https://www.drupal.org/project/taxonomy_manager/releases/2.0.11 →
I'm moving it to fixed. For term merge submodule issues let's open new issues.

This is a critical issue, I've discussed it with one more maintainer (Klausi) and we agreed to use a submodule approach.
I'll merge this PR and prepare a new release.

> A change this big should not be done in a patch-level release. This is a breaking change. Adding it as a submodule would be fine.
Well, a new major version sounds like a good approach. As it was said and if we check the official module info - https://www.drupal.org/project/taxonomy_manager → and
D7 version had this functionality without submodules (correct me if I'm wrong). And I think it makes sense to provide this support out of the box.
To me, it sounds like a missed feature request.

Well, based on D7 version and https://www.drupal.org/project/term_merge/issues/3160872 ✨ Integration with Taxonomy Manager Needs review it seems taxonomy_manager had built-in term merge integration, so I don't agree to move it to the separate module. Besides, we should add it to the taxonomy_manager module composer.json dependency.
However, it is an issue and the upgrade path should be tested.

Well, maybe we can move it to fixed?

Andriy Khomych → created an issue.

Attached patch from MR.

Attached patch

Andriy Khomych → changed the visibility of the branch 3443655-integration-with-term to hidden.

Here is MR - https://git.drupalcode.org/project/taxonomy_manager/-/merge_requests/28

Andriy Khomych → changed the visibility of the branch 3443655-integration-with-term to active.

Andriy Khomych → created an issue.

Attached patch.

Andriy Khomych → created an issue.

Hey Patrick Kenny, as it was said here https://www.drupal.org/project/feeds_ex/issues/3060977#comment-15081492 ✨ Allow overriding xpaths when creating feed Active
we have now a contrib module - https://www.drupal.org/project/feeds_ex/issues/3060977#comment-15081492 ✨ Allow overriding xpaths when creating feed Active
Feel free to check it and open issues on the related module page. And if you are fine with it, please, add related note to this issue.

I added a new patch, it has improved performance, and it was added as a commit to MR 23.

Here is a MR ready to be reviewed - https://git.drupalcode.org/project/term_merge/-/merge_requests/24

Andriy Khomych → changed the visibility of the branch 3013117-use-drupal-89 to hidden.