Account created on 28 June 2015, about 10 years ago
  • Senior Drupal Developer at Initlab 
#

Merge Requests

More

Recent comments

🇷🇺Russia walkingdexter

Closing due to lack of activity. Feel free to reopen if you can provide more information.

🇷🇺Russia walkingdexter

Closing due to lack of activity. Feel free to reopen if you can provide more information.

🇷🇺Russia walkingdexter

An alternative solution if you also want to ignore "access denied" messages.

🇷🇺Russia walkingdexter

@mvnovick thanks for the information! It looks like there is a conflict with one of the contrib or custom modules. Can you uninstall modules one by one until the error disappears?

🇷🇺Russia walkingdexter

Running performance_test.php with 55k entities.

Before:

 [info] Running memory usage tests:
 [info] Executing: /var/www/vendor/bin/drush batch-process 1238 --uri=http://example.com
>  [notice] Memory: 97.2 MB, non-peak mem: 90.5 MB
>  [notice] Memory: 122.45 MB, non-peak mem: 114.5 MB
>  [notice] Memory: 137.77 MB, non-peak mem: 127.5 MB
>  [notice] Memory: 150.63 MB, non-peak mem: 137.5 MB
>  [notice] Memory: 156.25 MB, non-peak mem: 143.5 MB
>  [notice] Memory: 166.26 MB, non-peak mem: 153.5 MB
>  [notice] Memory: 166.26 MB, non-peak mem: 153.5 MB
 [info] Generation completed in: 95.36 seconds
 [info] Executing: /var/www/vendor/bin/drush batch-process 1239 --uri=http://example.com
>  [notice] Memory: 94.5 MB, non-peak mem: 92.5 MB
>  [notice] Memory: 146.19 MB, non-peak mem: 135.5 MB
>  [notice] Memory: 168 MB, non-peak mem: 163.5 MB
 [info] Generation completed in: 44.71 seconds
 [info] Running query count tests:
 [info] Executing: /var/www/vendor/bin/drush batch-process 1240 --uri=http://example.com
>  [notice] Query count: 20212
>  [notice] Query count: 40563
>  [notice] Query count: 56204
>  [notice] Query count: 67989
>  [notice] Query count: 79562
>  [notice] Query count: 91456
>  [notice] Query count: 103885
>  [notice] Query count: 116314
>  [notice] Query count: 118477
 [info] Generation completed in: 127 seconds
 [info] Executing: /var/www/vendor/bin/drush batch-process 1241 --uri=http://example.com
>  [notice] Query count: 21904
>  [notice] Query count: 61511
>  [notice] Query count: 97748
>  [notice] Query count: 112545
 [info] Generation completed in: 52.29 seconds

After:

 [info] Running memory usage tests:
 [info] Executing: /var/www/vendor/bin/drush batch-process 1253 --uri=http://example.com
>  [notice] Memory: 72.5 MB, non-peak mem: 68.5 MB
>  [notice] Memory: 80.42 MB, non-peak mem: 72.5 MB
>  [notice] Memory: 84.71 MB, non-peak mem: 73 MB
>  [notice] Memory: 84.71 MB, non-peak mem: 73 MB
>  [notice] Memory: 89.24 MB, non-peak mem: 75 MB
>  [notice] Memory: 89.24 MB, non-peak mem: 75 MB
>  [notice] Memory: 89.24 MB, non-peak mem: 75 MB
 [info] Generation completed in: 93.93 seconds
 [info] Executing: /var/www/vendor/bin/drush batch-process 1254 --uri=http://example.com
>  [notice] Memory: 86.5 MB, non-peak mem: 84.5 MB
>  [notice] Memory: 97.25 MB, non-peak mem: 87 MB
>  [notice] Memory: 97.25 MB, non-peak mem: 89 MB
 [info] Generation completed in: 42.31 seconds
 [info] Running query count tests:
 [info] Executing: /var/www/vendor/bin/drush batch-process 1255 --uri=http://example.com
>  [notice] Query count: 20426
>  [notice] Query count: 42275
>  [notice] Query count: 55988
>  [notice] Query count: 67882
>  [notice] Query count: 79134
>  [notice] Query count: 91135
>  [notice] Query count: 103350
>  [notice] Query count: 115028
>  [notice] Query count: 118477
 [info] Generation completed in: 129.34 seconds
 [info] Executing: /var/www/vendor/bin/drush batch-process 1256 --uri=http://example.com
>  [notice] Query count: 22006
>  [notice] Query count: 64367
>  [notice] Query count: 103872
>  [notice] Query count: 112545
 [info] Generation completed in: 49.01 seconds
🇷🇺Russia walkingdexter

Need more information:

  1. Drupal version and PHP version.
  2. Do you have any custom or contrib modules installed that extend the Simple XML Sitemap module?
  3. Can you reproduce the error on a clean install?
🇷🇺Russia walkingdexter

Closing due to lack of activity. Feel free to reopen if you can provide more information.

🇷🇺Russia walkingdexter

I disagree with closing.
The problem is still there - loading all items at once.
It should be split into chunks or batches.

@dmitry.korhov Ok. Please provide a detailed explanation of how to see the benefit of the proposed changes. Also keep in mind that with these changes half of the links will not be included in the sitemap.

If you look closely at the current implementation of yieldItem(), you will see that the items are not loaded all at once, because we use fetchObject(). In the case of MySQL, the important thing is buffered queries. However, in my testing I found no correlation with memory consumption (Drupal 10.4, PHP 8.1, MySQL 8.0). The proposed changes also have no effect.

It's possible that the improvement has already been implemented at the core level. See 🐛 Implement statement classes using \Iterator to fix memory usage regression and prevent rewinding Fixed for details.

🇷🇺Russia walkingdexter

This is not an issue anymore (tested locally with performance_test.php). Yeah, the memory usage grows with the amount of data in the sitemap, but I couldn't find any correlation with yieldItem(), even if we select one element at a time from the queue. It must be related to something else.

🇷🇺Russia walkingdexter

@walkingdexter Would you mind documenting this ability in readme.md?

Done.

🇷🇺Russia walkingdexter

Added the current sitemap for context, removed the example of changing the entity's field as it is not obvious.

🇷🇺Russia walkingdexter

Closing due to lack of activity. Feel free to reopen if you can provide more information.

🇷🇺Russia walkingdexter

Closing due to lack of activity. Feel free to reopen if you can provide more information.

🇷🇺Russia walkingdexter

Closing due to lack of activity. Feel free to reopen if you can provide more information.

🇷🇺Russia walkingdexter

All our alter hooks are called ..._alter(). Why did you decide to use invokeAll() instead of alter() and call the hook hook_simple_sitemap_entity_process()?

Similar to entity hooks (load, update, insert, etc.). There is nothing to "alter" if a developer just wants to throw SkipElementException.

We also usually pass the current sitemap for context as secondary parameter.

Yeah, missed that. Definitely needs to be added.

Also I'm flirting with the idea to allow for users to set the entity to NULL inside the hook to skip it instead of them throwing our exception. Or allow both.

I don't see much benefit from this change. It can be confusing as there will be two ways to do the same thing. I think it's better to use the exception.

Finally, in simple_sitemap.api.php there is an example of changing the entity's field, however we don't pass the entity to constructPathData. Does this still work?

It works :) Most likely, this is due to the static entity cache.

🇷🇺Russia walkingdexter

@baikho thanks for the issue!

I like the idea, but not the implementation. This should be implemented similar to the Metatag module (per-entity overrides).

🇷🇺Russia walkingdexter

Looks like this is already in 4.x. Otherwise, a description must be added.

🇷🇺Russia walkingdexter

Created an issue in the Monitoring module and adapted the proposed code. See New sensor: Simple XML Sitemap Active for further work.

🇷🇺Russia walkingdexter

Please also save credits for those who worked on the original issue if the proposed changes are accepted.

🇷🇺Russia walkingdexter

This change should be mentioned in the release notes. The previous behavior can be restored using hook_entity_query_tag__simple_sitemap_alter() or hook_entity_query_tag__ENTITY_TYPE__simple_sitemap_alter().

Saved credits for @dieterholvoet as he participated in the discussion of this change in Discard adding if rabbit hole setting is active Needs review .

🇷🇺Russia walkingdexter
  • Added hook_simple_sitemap_entity_process() to allow other modules to process the entity.
  • Replaced event with hook to provide consistency and reduce support overhead. Replacing hooks with events should be discussed in a separate issue.
  • Support for the Rabbit Hole module must be implemented outside of this here module. It's difficult to create a general implementation that will satisfy all use cases. For example, the solutions presented here don't take into account user permissions that affect the behavior of the Rabbit Hole module.
🇷🇺Russia walkingdexter

Thanks for the issue! The proposed resolution is a breaking change, so I just fixed the info files.

🇷🇺Russia walkingdexter

@sokru I can't reproduce the specified error with Drupal 10.4.1 and Drush 12.5.3. Please provide more information about your environment.

🇷🇺Russia walkingdexter

We turned off path processing in order to remove language prefixed paths. Instead of having two half-solutions, we should find a way to disable these language prefixes without breaking aliases.

The original problem is already fixed in Drupal 10.3 🐛 Language negotiation breaks updating Drupal 9 to 10 Needs work . So the change from 🐛 Unexpected language prefixes on sitemap index Fixed can be reverted, but the core version requirement should be increased to 10.3. I think we don't have to worry about it because 10.2 is no longer supported. Just tested all the changes and everything works fine, including path aliases.

🇷🇺Russia walkingdexter

Please provide more information about the problem, steps to reproduce and proposed resolution. The current summary doesn't provide enough details to understand the situation.

🇷🇺Russia walkingdexter

I don't use commerce - what would the canonical link of a product variation be and why does it not have a canonical link template?

Product variation URLs depend on the parent product:
http://example.com/product/{commerce_product}?v={commerce_product_variation}

https://www.drupal.org/project/commerce/issues/2674888
https://www.drupal.org/project/commerce/issues/3025860

Do you mean to allow all content entities and then try-catch around the $entity->toUrl(); block if no URL is returned? Feel free to experiment with this.

I thought about it, but now I think it's better to use a simple solution. In a quick search, I couldn't find any other examples similar to product variations. So for now we have one entity type that requires special treatment.

🇷🇺Russia walkingdexter

@matthewv789 I can't reproduce this problem on a fresh install. Try composer why-not drupal/simple_sitemap 4.2.2 or composer require "drupal/simple_sitemap:4.2.2" --dry-run to get more details. Most likely the problem is related to your environment or Composer configuration.

🇷🇺Russia walkingdexter

@aarantes can you reproduce the problem on a clean install? For now it looks more like a problem with your environment.

🇷🇺Russia walkingdexter

The problem described here causes an error in Drupal 11. I think that's enough to bump the priority.

TypeError: Drupal\Component\Utility\Html::escape(): Argument #1 ($text) must be of type string, null given, called in /var/www/core/lib/Drupal/Component/Render/FormattableMarkup.php on line 238 in Drupal\Component\Utility\Html::escape() (line 431 of /var/www/core/lib/Drupal/Component/Utility/Html.php).

Rerolling the patch for 11.1.

🇷🇺Russia walkingdexter

The previous patch doesn't work with Composer due to version string. This one should work.

🇷🇺Russia walkingdexter

@iseeaflyingcrane Thanks for the issue! This is a valid point.

The reason for this behavior is that product variation does not have a canonical link template. However, if an entity doesn't have a canonical template, it can still have a canonical link. EntityHelper::supports() excludes entities that can be successfully added to the sitemap.

@gbyte Maybe we should support all content entity types? We can move entity types without a canonical link template to a separate table on the /admin/config/search/simplesitemap/entities page and warn users about possible errors.

🇷🇺Russia walkingdexter

The desired result can be achieved by using hook_entity_query_tag__TAG_alter or hook_entity_query_tag__ENTITY_TYPE__TAG_alter, where TAG is simple_sitemap. For example, for nodes the hook will be hook_entity_query_tag__node__simple_sitemap_alter. In this hook you can add a date condition to the query.

The above feature was added in Allow to alter entity url generator query Needs review and will be available in the next release.

🇷🇺Russia walkingdexter

See related issues for details.

🇷🇺Russia walkingdexter

Already fixed in Add Senzam search engine to IndexNow functionality Fixed . Marked this also as fixed to save credits.

🇷🇺Russia walkingdexter

Thanks for the clarification. This seems to be a very specific use case. If I understand correctly, the problem can be solved with a few lines of code using hook_simple_sitemap_links_alter(). Is this true?

🇷🇺Russia walkingdexter

I can't reproduce the problem by following these steps with Menu Item Extras 3.1.0. Please provide more information.

I believe this issue 🐛 option argument 'bundle' sometimes has a NULL value, which can cause issue Fixed covers a similar state, and they proposed to solve it by falling back to the entity type ID.

In this case the problem is more obvious, as the documentation says that FieldDefinitionInterface::getTargetBundle() can return NULL.

🇷🇺Russia walkingdexter

@WalkingDexter @dpi please check out the gitlab review. Let's not merge this before discussing if this should instead go into a 3rd party simple_sitemap_monitoring module which I feel should be the way to go ATM.

I think this functionality can be added directly to the Monitoring module. It already contains plugins for other contrib modules.

🇷🇺Russia walkingdexter

Merged, thanks!

This is not a breaking change because PHP is case insensitive for the class names. However, in some environments, errors may occur due to file name changes. See 💬 Class 'Drupal\simple_sitemap\Queue\SimpleSitemapQueue' not found Fixed for additional information. This should be described in the release notes.

🇷🇺Russia walkingdexter

Need more information about the use case. Why can't this be solved with the "Excluded languages" setting?

🇷🇺Russia walkingdexter

The proposed solution should not replace the current routing. It should be an option.

🇷🇺Russia walkingdexter

I don't see any practical use for this feature. This array is not intended to be modified. Feel free to reopen if you can provide a use case.

🇷🇺Russia walkingdexter

Need more information about the use case and the steps to reproduce.

🇷🇺Russia walkingdexter

The array of sitemap links should not be modified inside the addChunk() method.

🇷🇺Russia walkingdexter

8.x-3.x is outdated, it's unproductive to spend time on it. MR needs to be reworked for 4.x.

🇷🇺Russia walkingdexter

The necessary hooks are already in the core. All we need to do is add tag and metadata to the query.

🇷🇺Russia walkingdexter

I can't reproduce the problem on a clean install. Please provide steps to reproduce.

🇷🇺Russia walkingdexter

Committed, thanks!

ProcessOutbound Case:

Create a view with the URL of foo/bar/sitemap.xml. Try to go to this URL and you will be redirected to /bar/sitemap.xml.

FYI, that's not how it works. Outbound processors cannot be the cause of a redirect.

🇷🇺Russia walkingdexter

Need steps to reproduce. The queue is cleared before rebuilding.

🇷🇺Russia walkingdexter

Feel free to reopen if you have a problem with multisite installations.

🇷🇺Russia walkingdexter

Not related to 4.x, as sitemap types are not plugins.

🇷🇺Russia walkingdexter

The sitemap protocol still contains <priority> and <changefreq> values.

🇷🇺Russia walkingdexter

Fixed errors and merged, thanks! We can't use autoconfigure for now because it would break backwards compatibility.

🇷🇺Russia walkingdexter

My current thoughts:

  • In this issue we should focus on adding the ability to act on a processed entity. I propose to move the event subscriber (or hook implementation) to the Rabbit Hole module by creating a separate issue. Otherwise, I don't think it's right to drop support for Rabbit Hole 1. It's stable, and we'll have to support both versions.
  • The discussion about events and hooks is relevant again because of this change record .
  • The proposed code must use all possible type declarations.
  • All warnings from the CI pipeline must be fixed.
Production build 0.71.5 2024