🇨🇦Canada @adam-vessey

Rerolled patch for/against group 2.3.0.

Will put together an MR.

@ion.macaria: The alter is invoked for all entity queries, but contains logic to exit early without modifying the query when there are no relevant entity types. It is in determining if there are any relevant entity types that the slow query is located, which can be improved by the memoization/caching in the patch from #2

@stevenjay: Disabling SQL rewriting could affect/suppress other access control mechanism that might be expected to also alter the queries. If the view in question is exclusively for administrative uses, perhaps? Otherwise, can be risky.

@mxh: Meaning the `$plugin_ids_used` bit on https://git.drupalcode.org/project/group/-/blob/2.3.x/group.module?ref_t... / https://git.drupalcode.org/project/group/-/blob/3.3.x/group.module?ref_t... ? I vaguely recall looking at it, but I'm wanting to say that it was less of an issue with the additional `entity_id` condition it contains and possibly further mitigated by the expectation of the access check being cacheable; however, it has been a while since I've looked at it.

In terms of other data/schema types, I suspect all of those `list_*` types provided by the core "options" modules would be similarly problematic, so in addition to `list_string`, also `list_integer` and `list_float`? That said, I've not (yet) verified them personally.

This seems related to https://www.drupal.org/project/monolog/issues/3346731 🐛 Functional test error service not found for one of the monolog services. Active , where the issue is more one of attempting to reference services provided by modules in general during site installation, where 3346731 was attempting to make use of dblog's logger.dblog service.

In terms of what I used in our `settings.php`, something like:

if (!\Drupal\Core\Installer\InstallerKernel::installationAttempted()) {
  $GLOBALS['conf']['container_service_providers'][] = \Drush\Drupal\DrushLoggerServiceProvider::class;
  $settings['container_yamls'][] = __DIR__ . '/monolog.services.yaml';
}

Appeared to do the trick, but not sure how or where it would make sense to tie this into the documentation.

adam-vessey → created an issue.

Should jFrog Artifactory instead just be made aware of Drupal's Composer repo? I'm not familiar with anything jFrog, but a quick look around their docs indicate that they support adding additional repositories: https://jfrog.com/help/r/jfrog-artifactory-documentation/set-composer-re...

adam-vessey → created an issue.

adam-vessey → created an issue.

After updating `flysystem_s3`, one of our custom modules which extends Drupal\flysystem_s3\Flysystem\S3 starting throwing some of the "deprecated: dynamic property" warnings due trying to set against an instance of the upstream class instead of the static bound class.

MR from #2 (at e9995f3f8a1db1f2aef0d37ccb7dce0a7184406b) seems to do the trick to get rid of the warning (and restore the behaviours from the extending code).

Slapped together a patch introducing a naive bit of memoization/caching which seems to do the trick; however, not entirely a fan of it. Half-thinking that this query and caching thereof could make sense being moved up to a/the plugin manager?

Also, unsure of particular cache bins to use (went with `cache.data`), and cache key/id, but was sufficient to get from >30 second pages loads down to ~3 seconds (still more to pursue elsewhere).

adam-vessey → created an issue.

On further investigation, it looks like it was breaking things in other weird and wonderful ways, such that `updb` and the like would break due to route rebuilding, so maybe the "advanced usage" we were attempting is just not supported? Moving away from the additional extension pass, so this would no longer be an issue.

That said, removing/deprecating these constants may still be a good idea?

Presently, we have elected to patch around this by jamming a test in the top of `search_api_fast.module`; however, this may result in these constants in question being left unset due to the use of `include_once` being used to load the `.module` files.

The "test" in question, attached, essentially just a:

if (!\Drupal::hasContainer()) {
  return;
}

at the top of the file, as the .module does nothing else than define these constants, presently.

adam-vessey → created an issue.

@darvanen: The ::groupExists() method is a callback for the machine name element of the form ( https://git.drupalcode.org/project/context_groups/-/blob/fc7d8209b00a851... ), and as such is more concerned with determining if a given name is unique on the given site. Uniqueness can't really be evaluated if some results are filtered from the query, so ::accessCheck(FALSE) probably makes the most sense; otherwise, it might be possible for different users who can't see each others contexts to bind the same name, and make a mess for _other_ users who can see both? Otherwise, it seems like we would have to get into some kind of namespacing to define "uniqueness" domains? Seems like something of a rather larger overhaul of the module than we're looking to do here.

As for the method's visibility, I've gotta admit that PHP's callable/callback-passing semantics are a little fuzzy to me: If it _was_ defined as protected/private, would it be usable as a callback for the machine name element? Looks like there's a call to the "exists" callback in MachineName::validateMachineName() ( https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Render%21... ), which means that it would be trying to call this method that it strictly should not be able to see, but that it was passed by something that _can_ see it? Will the callback expressed as an array of object and method, I'm not sure that visibility could correctly be resolved... maybe it would be ignored? Bit of naive testing indicates that it is not possible:

<?php
class A {
  public function methodOne() {
    echo "one... ";
    return [$this, 'methodTwo'];
  }

  protected function methodTwo() {
    echo "two... ";
  }
}

$instance = new A();

$callable = $instance->methodOne();
call_user_func($callable);

Resulting in:

one... 
Fatal error: Uncaught TypeError: call_user_func(): Argument #1 ($callback) must be a valid callback, cannot access protected method A::methodTwo() in /home/user/scripts/code.php:16
Stack trace:
#0 {main}
  thrown in /home/user/scripts/code.php on line 16

With the newer first-class callback syntax introduced in PHP 8.1 (with $this->methodTwo(...) instead of [$this, 'methodTwo']), it _does_ appear to be possible; however, such seems to be getting rather outside of the intended scope of this issue.

adam-vessey → created an issue.

adam-vessey → created an issue.

Appears to do the trick.

Was getting this deprecation message just when doing a config exports and imports via drush. Threw this in (patch-wise), and am no longer getting the deprecation message.

Gave #3 a spin, and it seems to work, here. Thanks!

Bit of poking around, looks like this might be actually be an expectation of PHP proper, that a public $context property exists on stream wrapper implementations: https://www.php.net/manual/en/class.streamwrapper.php#streamwrapper.prop...

It looks like it's been there for a _long_ time (as in, added in PHP 5 days, according to the wayback machine http://web.archive.org/web/20150814000527/http://php.net/manual/en/class... ), and is only being exposed now with the bump to PHP 8.2 being more strict with dynamic properties?

That said, might make more sense to just define the:

public $context;

bit, instead of allowing any arbitrary __set(); though, on second though, we subclass Twistor\FlysystemStreamWrapper, which would have the same issue?: https://github.com/twistor/flysystem-stream-wrapper/issues/27

I threw together https://github.com/twistor/flysystem-stream-wrapper/pull/28 to get try to get it fixed upstream.

Gave things a bit of a run over on D10 proper.

Test procedure, approximately:
- up'd D10 ddev env
- `ddev composer require mglaman/composer-drupal-lenient cweagans/composer-patches`
- `ddev composer config --merge --json extra.drupal-lenient.allowed-list '["drupal/migrate_directory"]'`
- `ddev composer config --merge --json extra.patches '{"drupal/migrate_directory": {"D10 Supports": "https://www.drupal.org/files/issues/2022-06-16/migrate_directory.2.0.0.rector.patch"}}'`
- `ddev composer require drupal/migrate_directory`
- added a migration to a module (migrate_directory, in my testing, creating it a `migrations` directory, with a `test_migration.yml`):
    ```
    ---
    id: test_migration
    label: Test Migration
    source:
      plugin: directory
      path: .
    destination:
      plugin: "null"
    ```
- ran bit of test code to check if things appear to be iterating in any capacity:
    ```
    <?php

    /** @var \Drupal\migrate\Plugin\MigrationPluginManagerInterface $pmm */
    $pmm = \Drupal::service('plugin.manager.migration');
    $test_migration = $pmm->createInstance('test_migration');

    /** @var \Drupal\migrate\Plugin\MigrateSourcePluginManager $pmms */
    $pmms = \Drupal::service('plugin.manager.migrate.source');

    $instance = $pmms->createInstance('directory', [
      'path' => '.',
    ], $test_migration);

    var_dump(iterator_count($instance));

    $instance = $pmms->createInstance('directory', [
      'path' => '.',
      'pattern' => '/.twig/',
    ], $test_migration);

    var_dump(iterator_count($instance));

    ```
    - output:
        ```
        int(16567)
        int(970)
        ```

Very much a synthetic test, just seeing that it appears to iterate over _something_ and that it can filter, but looks like things should be fine for D10.

I gave a look at this in a ddev D10 env, and it indeed installed (after adding the repository entry and requiring the module accordingly); however, I found that there may be some styling updates desirable? As in: It seems like the flat 20% width in the CSS (https://git.drupalcode.org/project/betterlogin/-/blob/dfdd367b3f3937a01b...) causes overflow issues, with the username/password/etc fields being partially hidden with the default configuration.

That said, further examined our internal environment and found that we weren't actually using this module despite having it installed and enabled as gin_login was taking precedence; so in our instance, moving to remove betterlogin from our environment.

I've tested this out against D10, and it seems to work.

Procedure employed:

1. Up'd base D10 ddev env using instructions: https://ddev.readthedocs.io/en/latest/users/quickstart/#drupal
2. Change minimum-stability to dev
   • prefer-stable is left as true
3. Added adjusted repo entries more or less according to #3297257-28: Automated Drupal 10 compatibility fixes → , but with canonical to false on each entry
4. ddev composer require "drupal/flysystem_s3:dev-3297257-automated-drupal-10 as 2.x-dev"
5. configured $settings['flysystem'] in web/sites/default/settings.php (with the test-thing scheme)
   • made use of a key/secret ("borrowed" from an existing env)

   •     $settings['flysystem'] = [
           'test-thing' => [
             'driver' => 's3',
             'config' => [
               [... snipped key, secret, region, bucket, public and prefix values ...]
             ],
             'cache' => TRUE,
           ],
         ];
         

6. ddev drush en flysystem_s3
7. ddev drush cr ('cause paranoia)
8. ddev drush php-eval "var_dump(file_put_contents('test-thing://some-dir/file.txt', 'what'));"
   Output: int(4)
9. ddev drush php-eval "var_dump(file_get_contents('test-thing://some-dir/file.txt'));"
   Output: string(4) "what"

So, the write and read via PHP appears to work. Also, checked the S3 console to confirm that the file was created where it was expected, and it was. Tempted to flip this over to RTBC; however, I didn't verify the CORS functionality, so will let somebody else make the call. That said, the core read/write seems functional in D10.

Thinking something like this should do the trick?

adam-vessey → created an issue.

Thinking something like this should do the trick?

adam-vessey → created an issue.

Thinking something like this should do the trick?

adam-vessey → created an issue.

adam-vessey → created an issue.

Err... I... might've accidentally done a thing? I didn't intend to create the issue fork/branch? I sas just trying to post a comment indicating that adding package entries for citation-style-language/locales (and [..]/styles) should no longer be necessary, as they appear to have Packagist releases (as of March 2023?).

adam-vessey → made their first commit to this issue’s fork.

Just noticed the stray string `advanced_help`

in the example mapping, introduced in the latest revision. With context:

    'grddl'    => 'http://www.w3.org/2003/g/data-view#',
    'ma'       => 'http://www.w3.org/ns/ma-ont#',advanced_help
    'owl'      => 'http://www.w3.org/2002/07/owl#',

Suspecting it probably shouldn't be there?

Hit the "rebase" button on the MR, to deal with that for now.

The MR seems to do the trick to deal with my "unique violation" exception; however, holding off on swapping the status to RTBC as it does not quite match the original described scenario; though, might only be a slight alteration:

To the "Scenario", making the view process batch-wise, processing a single item per batch ("Process in a batch operation" checked, and "Batch size" of 1; I could not get this to happen when _not_ processing in a batch)

And for "Observed", unique paragraphs being created for all items in the first batch set, but then the "unique violation" exception being thrown adding to the first item in the second batch set (the "batch size + 1"th item).

Seems to suggest something being serialized in the batch state that possibly/probably shouldn't be, but the ::createDuplicate/clone procedure deals with it.

Just hiding the patch, as it's cannot presently be applied.

Actually, scratch that, I was looking just at the patch, not the code in fork. Having taken a quick look at the fork, I thought the fork's claiming that it still had to be rebased was the same thing, but it _does_ actually target the BulkEditFormTrait.

Things are not mergeable as-is, as #3173187: Make VBO an optional dependency → / https://git.drupalcode.org/project/views_bulk_edit/-/commit/55f630241d71... moved the code in question.

Got looking into this, as we have run into issues when attempting to manipulate replace/add a number of paragraph entities greater than the batch size, and encountering SQL exceptions (using PostgreSQL... is MySQL less severe with "unique" collisions?). Exception as such:

Drupal\Core\Entity\EntityStorageException: SQLSTATE[23505]: Unique violation: 7 ERROR: duplicate key value violates unique constraint "paragraphs_item__paragraph_field__uuid__value__key" DETAIL: Key (uuid)=(69262fed-58ba-4021-8a2f-edebfbd7befa) already exists.: INSERT INTO "paragraphs_item" ("revision_id", "type", "uuid", "langcode") VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3) RETURNING id; Array ( ) in Drupal\Core\Entity\Sql\SqlContentEntityStorage->save() (line 815 of /opt/www/drupal/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php).

This would only pop when going to process the first item in the second batch of items.

Had this in a private fork on GitHub, and have been asked to submit it as a patch, so here it is:

Looking back on this, most of the values in the `rest_oai_pmh.settings` object are still undefined.