🇷🇺Russia @WalkingDexter

Closed due to lack of activity.

This feature cannot be implemented in a general way for arbitrary links, #6 is the best solution.

Closed because 3.x is unsupported. Feel free to reopen if the problem is present in 4.x.

Closed due to lack of activity.

Closed because 3.x is unsupported. Feel free to reopen if the problem is present in 4.x.

Closed because 3.x is unsupported. Feel free to reopen if the problem is present in 4.x.

I still see a lot of unnecessary changes that are not related to the issue. Please explain why these changes are needed. In my opinion, the only changes we need here are the changes that Project-Update-Bot suggests. Also we cannot rely on DeprecationHelper as it's available since version 10.1.3 (see the change record → ). We must either drop support for Drupal 9 or implement a workaround.

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

The changes made break the value options. Attached the correct patch.

I'm not sure about these changes:

• https://git.drupalcode.org/project/simple_sitemap/-/merge_requests/90/di...
• https://git.drupalcode.org/project/simple_sitemap/-/merge_requests/90/di...

They need to be carefully reviewed. At least there is no need to change the file mode. Additionally, any errors from the validate step should be fixed.

These phpcs violations should be fixed in 📌 Automated Drupal 11 compatibility fixes for simple_sitemap Needs review . I also see a lot of unnecessary changes (like file mode) and ignoring the current phpcs configuration.

The standard is the living Coder project.

OK, using short class names in hook signatures is not a violation according to Coder.

No changes are needed to the module. There is a coding standard → .

API documentation (in .api.php files) should use full class names. Note that if a class is used more than once in multiple hook signatures, it must still be "use"ed, and then only the short names of the class should be used in the function.

There is no "message" key in the 2.x. It's only available in the 1.x and the upgrade path is not specified. You need to reinstall the module and configure it again.

Patch with proposed resolution.

WalkingDexter → created an issue.

This module still supports Drupal 9 where ByteSizeMarkup::create() is not present. Until this support is dropped, this issue requires a workaround.

Already fixed in 4.x.

First of all, PHPStan errors need to be fixed https://git.drupalcode.org/issue/simple_sitemap-3432628/-/jobs/1121610.

WalkingDexter → created an issue.

WalkingDexter → created an issue.

WalkingDexter → created an issue.

Stopped at level 5 (max level for Drupal projects for now). If someone wants to fix errors from the baseline, they should do it in a separate issue.

WalkingDexter → created an issue.

Maybe this information will be useful.

The recommended approach to installing PHP extensions from GitLab CI guide → doesn't work with drupalci/php-8.2-apache:production.

MongoDB example:

$ pecl install mongodb
WARNING: channel "pecl.php.net" has updated its protocols, use "pecl channel-update pecl.php.net" to update
downloading mongodb-1.17.2.tgz ...
Starting to download mongodb-1.17.2.tgz (2,064,433 bytes)
......................................................................................................................................................................................................................................................................................................................................................................................................................done: 2,064,433 bytes
846 source files, building
running: phpize
Configuring for:
PHP Api Version:         20220829
Zend Module Api No:      20220829
Zend Extension Api No:   420220829
Cannot find autoconf. Please check your autoconf installation and the
$PHP_AUTOCONF environment variable. Then, rerun this script.
ERROR: `phpize' failed

I also don't see any option to install a bundled PHP extension. For example, imap is available in pecl only since PHP 8.3.

Should the status be "Fixed" instead of "Postponed"?

Merged, thanks!

Patch #4 works for me (version 2.0.2)

WalkingDexter → created an issue.

+1 RTBC

WalkingDexter → created an issue.

WalkingDexter → created an issue.

Meantime the error points out that the script attempted to extract php source code vut that's wrong action - instead of rebuilding PHP itself it should use installed headers instead.

As I can see in the install-php-extensions script the error comes from docker-php-source extract (standard utility in official PHP images). PHP sources are used only to get the list of bundled extensions.

Contrib willing to install something must care about how to build this extras as the debian base image for different php versions vary

The install-php-extensions script already supports a variety of Debian-based and Alpine-based images, as well as various PHP versions. Until recently it was a good solution to install PHP extensions (without additional overhead) in drupalci images too.

Is it possible to revert the change that deletes the /usr/src/php.tar.xz file? Or it's a major change and it's not for discussion? Some other useful scripts compatible with PHP-based images may rely on docker-php-source extract, and the above change may break them too.

Most likely, the above changes are the cause of the problem described below.

Installation of additional PHP extensions via mlocati/docker-php-extension-installer (easy installation of extensions with their deps) fails with an error when using the image drupalci/php-8.2-apache:production.

tar (child): /usr/src/php.tar.xz: Cannot open: No such file or directory
tar (child): Error is not recoverable: exiting now
tar: Child returned status 2
tar: Error is not recoverable: exiting now

The error doesn't occur when using images drupalci/php-8.0-apache:production, drupalci/php-8.1-apache:production and drupalci/php-8.2-apache:production-old.

I'm not sure if this is the appropriate place for this problem. However, someone may encounter the same error when setting up a GitLab CI configuration for their contrib project, which requires the installation of additional PHP extensions.

Thanks!

Example:

custom.services.yml

services:
  custom.event_subscriber:
    class: Drupal\custom\EventSubscriber\CustomEventSubscriber
    arguments: ['@messenger']
    tags:
      - { name: event_subscriber }

src/EventSubscriber/CustomEventSubscriber.php

<?php

namespace Drupal\custom\EventSubscriber;

use Drupal\content_translation_redirect\ContentTranslationRedirectEvents;
use Drupal\content_translation_redirect\Event\ContentTranslationRedirectEvent;
use Drupal\Core\Messenger\MessengerInterface;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;

/**
 * Custom event subscriber.
 */
class CustomEventSubscriber implements EventSubscriberInterface {

  /**
   * The messenger.
   *
   * @var \Drupal\Core\Messenger\MessengerInterface
   */
  protected $messenger;

  /**
   * CustomEventSubscriber constructor.
   *
   * @param \Drupal\Core\Messenger\MessengerInterface $messenger
   *   The messenger.
   */
  public function __construct(MessengerInterface $messenger) {
    $this->messenger = $messenger;
  }

  /**
   * Handles the content translation redirect.
   *
   * @param \Drupal\content_translation_redirect\Event\ContentTranslationRedirectEvent $event
   *   The event to process.
   */
  public function onRedirect(ContentTranslationRedirectEvent $event): void {
    $this->messenger->addStatus('Your message is here');
  }

  /**
   * {@inheritdoc}
   */
  public static function getSubscribedEvents(): array {
    $events[ContentTranslationRedirectEvents::REDIRECT][] = ['onRedirect'];
    return $events;
  }

}

Rebased the source branch, now all tests are passed. The test-only results now show the failure for core/tests/Drupal/KernelTests/Core/Entity/EntityAutocompleteTest.php.

@gbyte I don't think this is possible since the commit is already done. Don't worry, my contribution is already listed in the Git message.

@daniel.bosen Thank you for reporting! MR has changes in the wrong place, but the main idea is correct. I fixed the problem in the above commit.

@gbyte This bug is a symptom of a bigger problem with SitemapGetterTrait. The list of set sitemaps is statically cached in the $sitemaps property. Sometimes this leads to non-obvious results if some code previously called the setSitemaps() method and specified a parameter other than NULL. In other words, in some cases the getSitemaps() method is expected to return a list of all compatible sitemaps, but instead it returns only those that were previously set.

@Rajeshreeputra This won't work. Your changes suggest that the $variant variable will have different values depending on the PHP version. Also checking the PHP version at runtime is an overhead in our case.

WalkingDexter → created an issue.

@gbyte Our core_version_requirement key says that the module requires Drupal 9.3.0 or later. The minimum PHP version for 9.3.0 is 7.3. Yeah, it's not recommended → anymore, but there may still be Drupal 9 sites that use PHP 7.3 or PHP 7.4.

I see the following solutions:

• Increase core_version_requirement to ^10.
• Don't use PHP 8 features (like the Nullsafe operator) while the module supports Drupal 9.
• Set PHP version requirement for the module.

WalkingDexter → created an issue.

The error is related to the domain_simple_sitemap module (read the error message carefully). This module is not compatible with 4.x version. See this issue for details ✨ Make compatible with simple_sitemap 4.0 & domain_entity Needs review .

The patch is not needed. If $form_object->getOperation() returns NULL it means that the EntityFormInterface is not implemented correctly.

Hello, @hartsak!

The message feature was removed for a reason - it's difficult to cover all possible use cases. In my opinion, this feature should be at the custom code level. I'm planning to add an event or hook before the redirect in 2.x, so that any desired behavior can be implemented. Until it's done, I recommend to use 1.x if it suits your requirements. The 2.x version has some nice new features (but they are not finalized yet) and if you need them you can implement a patch that returns the message feature.

Prevent double sanitization.

Fixed in 2.x. All issue credits are saved.

Thanks! Committed with changes and additions. The module must be reinstalled due to configuration changes.

I can't reproduce the problem on a fresh install. FYI, the weight specified in the simple_sitemap_entity_extra_field_info() is the default weight. When you save the form display the weights are updated. Removing the function is definitely not a solution to the problem. This function was added for a reason #3176344: Add sitemap settings to entity forms using hook_entity_extra_field_info → . Please provide steps to reproduce the problem on a fresh install.

Improvements for tests.

I removed [] from the list of empty values because it causes an error:

1) Drupal\KernelTests\Core\Entity\EntityAutocompleteTest::testEntityReferenceAutocompletion
Symfony\Component\HttpFoundation\Exception\BadRequestException: Input value "q" contains a non-scalar value.

/var/www/web/vendor/symfony/http-foundation/InputBag.php:37
/var/www/web/core/modules/system/src/Controller/EntityAutocompleteController.php:82
/var/www/web/core/tests/Drupal/KernelTests/Core/Entity/EntityAutocompleteTest.php:209
/var/www/web/core/tests/Drupal/KernelTests/Core/Entity/EntityAutocompleteTest.php:142
/var/www/web/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

@smustgrave Here it is.

Good points! I updated the patch with minor changes - empty() is not necessary when the variable is defined.

If someone needs similar improvements.

WalkingDexter → created an issue.

The patch is not needed if the Fast Token Browser → module is used.

gbyte → credited WalkingDexter → .

Patch with tests.

Fix for terms without core patch (Drupal 7).

/**
 * Implements hook_field_widget_WIDGET_TYPE_form_alter().
 */
function custom_field_widget_taxonomy_autocomplete_form_alter(&$element, &$form_state, $context) {
  array_unshift($element['#element_validate'], 'custom_taxonomy_autocomplete_validate');
}

/**
 * Form element validate handler for taxonomy term autocomplete element.
 *
 * @param array $element
 *   The element structure.
 */
function custom_taxonomy_autocomplete_validate(array &$element) {
  // Fix for single term with name "0".
  if ($element['#value'] === '0') {
    $element['#value'] .= ' ';
  }
}

Drupal 7 also has this problem with taxonomy autocomplete.

WalkingDexter → created an issue.

@gbyte What do you think?

@arti_parmar It makes sense, thanks for pointing this out.

I added the ability to redirect to the specified path. Use / to redirect to the front page. Thanks!

I believe right now it is only stored temporarily in \Drupal\simple_sitemap\Queue\QueueWorker::stashResults() along with the rest of the link data when the generation process does not complete during given time... Let me know if you think meta information is stored in the database anywhere else.

My concerns relate specifically to this method. \Drupal\simple_sitemap\Queue\QueueWorker::stashResults() uses the state system to store information. Default implementation of \Drupal\Core\State\StateInterface uses the key/value store. Default implementation of \Drupal\Core\KeyValueStore\KeyValueStoreInterface uses the database to store key/value data.

\Drupal\Core\State\State - default state implementation.
\Drupal\Core\KeyValueStore\DatabaseStorage - default key/value store implementation.

One way or another, in some cases the data is stored in the database during the sitemap generation. Storing heavy data (entity objects) should be justified and at least properly tested. At this moment, I think it more reasonable to store the entity ID and load the object on demand. Note that the entity static cache prevents the same entities from being reloaded in the same request.

This is a fair point and seeing the big code overhead events create, I am reluctant to switch to it. Which one do you prefer for this module?

Usually I prefer to use events because they are more flexible. If we want to use events, then we should introduce them in the next major update, since we cannot simply remove hooks due to backwards compatibility.

@gbyte I agree with #16. Meta information is stored in the database, most likely the proposed change will create memory issues.

This hook working during generation but not during queue creation might be confusing for implementers ("Why do only some entities go through this hook?"). This could be circumvented by removing the following from EntityUrlGenerator::getDataSets thus delegating access checks completely to the generation process

Not sure about this change because I don't fully know the queue processing logic.

But yeah, this sounds cleaner indeed. I'm not a big fan of hooks either hence this module's extensive API. Just wondering how much code overhead it will be to implement reacting to that event for e.g. the rabbit_hole module as opposed to a hook implementation.

And I'm not a big fan of mixing hooks and events :) I think we should choose one of these approaches for the whole API.

...take a look at the code and state your opinion about all of it

Is it a good idea to remove the subscriber definition when the rabbit_hole module is not installed? Maybe it's better to add the definition when the module is installed?