🇫🇮Finland @kekkis

#13 works here too, but agreed with berdir that a more elegant solution would not hurt.

Removed unnecessary logging.

Adding latest version of MR !23 as a patch file for composer use. It applies cleanly on 8.x-1.4.

Added compatibility with PHP 8.2 by not having properties created dynamically in the constructor.

Thanks @kallevu for the update on the MR! I tried working on that on the global contrib weekend but failed miserably. It would be nice if this could move forward now that it seems to produce green results too.

See related issue, the patch there fixes this particular problem. There are other problems in that plugin though, but that is out of scope for this issue per se.

Updated MR#89 to use a new, higher (+1) schema version since https://www.drupal.org/project/group/releases/2.3.1 → introduced group_update_9211.

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

Added the issue I encountered as a related issue although I recognized it's not actually an issue with this change, rather with another MR sharing the hook ID.

Apparently it is present in MR 89. Now I should find that one. Anyway, not a new issue. Closing.

Apologies, this was caused by a patch that we are using. I'm trying to identify the patch to fix it.

kekkis → created an issue.

The group_update_9211 function is now defined twice in group.install in version 2.3.1. I'll create a follow-up.

LGTM!

LGTM here too. On both MRs.

kekkis → changed the visibility of the branch 2.1.x to hidden.

kekkis → changed the visibility of the branch 2.2.x to hidden.

Looks good to me.

kekkis → created an issue.

Manually tested the MR using the drush command with:

1. drupal/upgrade_status: 4.3.5
2. mglaman/phpstan-drupal: 1.3.1
3. palantirnet/drupal-rector: 0.20.3

Found no issues.

I tried adding test support using Gitlab CI in #3481075 📌 Setup GitLab CI pipelines for phpcs, phpstan, eslint, composer validations Active but seems I ran out of competence and/or time. Will need to dive deeper into that at some point.

However, I would like to propose that we merge this and release it as part of the 2.1.1 release which is also going to introduce basic Drupal 11 support, albeit not for the tests.

Opinions for or against?

Well, now tests are not failing because of D11 support anymore. However there are multiple confusing errors when running even just phpstan locally. It seems the tests are fundamentally incompatible with D11. Next steps perhaps: look into how modules have kept supporting D10 and D11 with the test base class changes. (Or prove me wrong in my analysis.)

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

Hmm, tests are failing because the module has no support for D11 yet. Let's fix that.

MR created.

kekkis → created an issue.

Thanks, project update bot!

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

MR approved so RTBC.

heikkiy → credited kekkis → .

Updated issue summary.

@japerry, I have pushed changes in the MR now. Please review again. As you can see in the comments, I also manually tested with the two combos that IMO make some kind of sense: Drupal 10.1+Drush 11 and Drupal 10.2+Drush 12. But as I wrote this note, I thought of yet another combo that is sensible in some cases: Drupal 10.1 + Drush 12. Here's the output for that test, which also indicates that the combination works ok with the branch:

$ ddev drush -vvv p:diagnostics
 [preflight] Config paths: /var/www/html/vendor/drush/drush/drush.yml
 [preflight] Alias paths: /var/www/html/web/drush/sites,/var/www/html/drush/sites
 [preflight] Commandfile search paths: /var/www/html/vendor/drush/drush/src
 [debug] Bootstrap further to find p:diagnostics [0.32 sec, 2.63 MB]
 [debug] Trying to bootstrap as far as we can [0.32 sec, 2.63 MB]
 [info] Drush bootstrap phase: bootstrapDrupalRoot() [0.32 sec, 2.64 MB]
 [info] Change working directory to /var/www/html/web [0.32 sec, 2.64 MB]
 [info] Initialized Drupal 10.1.8 root directory at /var/www/html/web [0.32 sec, 2.64 MB]
 [info] Drush bootstrap phase: bootstrapDrupalSite() [0.32 sec, 2.77 MB]
 [debug] Could not find a Drush config file at sites/default/drush.yml. [0.32 sec, 2.84 MB]
 [info] Initialized Drupal site purge-test-10-1-drush-12.ddev.site at sites/default [0.32 sec, 2.84 MB]
 [info] Drush bootstrap phase: bootstrapDrupalConfiguration() [0.32 sec, 2.84 MB]
 [info] Drush bootstrap phase: bootstrapDrupalDatabase() [0.33 sec, 3.03 MB]
 [info] Successfully connected to the Drupal database. [0.33 sec, 3.02 MB]
 [info] Drush bootstrap phase: bootstrapDrupalFull() [0.33 sec, 3.03 MB]
 [debug] Start bootstrap of the Drupal Kernel. [0.33 sec, 3.02 MB]
 [debug] Finished bootstrap of the Drupal Kernel. [0.35 sec, 3.7 MB]
 [debug] Loading drupal module drush commands & etc. [0.35 sec, 3.7 MB]
 [debug] Found drush.services.yml for purge Drush commands [0.35 sec, 3.71 MB]
 [debug] Add a commandfile class: Drush\Drupal\Commands\sql\SanitizeCommands [0.37 sec, 4.52 MB]
 [debug] Add a commandfile class: Drush\Drupal\Commands\sql\SanitizeCommentsCommands [0.37 sec, 4.52 MB]
 [debug] Add a commandfile class: Drush\Drupal\Commands\sql\SanitizeSessionsCommands [0.37 sec, 4.52 MB]
 [debug] Add a commandfile class: Drush\Drupal\Commands\sql\SanitizeUserFieldsCommands [0.37 sec, 4.53 MB]
 [debug] Add a commandfile class: Drush\Drupal\Commands\sql\SanitizeUserTableCommands [0.37 sec, 4.54 MB]
 [debug] Add a commandfile class: Drupal\purge\Drush\Commands\DebugCommands [0.37 sec, 4.54 MB]
 [debug] Add a commandfile class: Drupal\purge\Drush\Commands\DiagnosticsCommand [0.37 sec, 4.55 MB]
 [debug] Add a commandfile class: Drupal\purge\Drush\Commands\InvalidateCommand [0.37 sec, 4.55 MB]
 [debug] Add a commandfile class: Drupal\purge\Drush\Commands\ProcessorCommands [0.37 sec, 4.56 MB]
 [debug] Add a commandfile class: Drupal\purge\Drush\Commands\PurgerCommands [0.37 sec, 4.58 MB]
 [debug] Add a commandfile class: Drupal\purge\Drush\Commands\QueueCommands [0.37 sec, 4.61 MB]
 [debug] Add a commandfile class: Drupal\purge\Drush\Commands\QueuerCommands [0.37 sec, 4.64 MB]
 [debug] Add a commandfile class: Drupal\purge\Drush\Commands\TypesCommand [0.37 sec, 4.65 MB]
 [debug] Done with bootstrap max in Application::bootstrapAndFind(): trying to find p:diagnostics again. [0.42 sec, 6.97 MB]
 [info] Starting bootstrap to none [0.43 sec, 7.09 MB]
 [info] Drush bootstrap phase 0 [0.43 sec, 7.09 MB]
 [info] Try to validate bootstrap phase 0 [0.43 sec, 7.09 MB]
 [error]  diagnostics: ERROR: Purgers: There is no purger loaded which means that you need a module enabled to provide a purger plugin to clear your external cache or CDN. [0.44 sec, 7.45 MB]
 -------------------- ------------------------------------------------------------------------- ---------------------------------------- ----------
  Title                Recommendation                                                            Value                                    Severity
 -------------------- ------------------------------------------------------------------------- ---------------------------------------- ----------
  Queuers              Queuer for the 'drush p:queue-add' command.                               Drush p:queue-add                        OK
  Page cache max age   Your site instructs external caching systems not to cache anything. Not   no caching                               WARNING
                       only does this make cache invalidation futile, it is also a great
                       danger to your website as any form of traffic can bring it down
                       quickly!
  Capacity             There is no purging capacity available.                                   0                                        WARNING
  Queue size           Your queue is empty!                                                      0                                        OK
  Purgers              There is no purger loaded which means that you need a module enabled to                                            ERROR
                       provide a purger plugin to clear your external cache or CDN.
  Processors           You have multiple processors working the queue.                           Drush p:queue-work, Drush p:invalidate   OK
 -------------------- ------------------------------------------------------------------------- ---------------------------------------- ----------

Thanks, committed to 2.1.x!

@kalpanajaiswal, thanks for your contribution! As I mention in the MR comment, I don't see value in adding a .gitlab-ci.yml that is so generic it only runs sleep. I would suggest we remove it from the MR branch.

kekkis → created an issue.

HeikkiY → credited kekkis → .

I have now created two new branches, one based on 11.x and another on 10.2.x. Attaching patch for 10.2.x too (applies cleanly to 10.1.x as well). Leaving as NW since I haven't (yet) had time to add tests.

I made the choice to continue work on the patch at #70 instead of #67 since it seemed to be the more complete solution.

kekkis → changed the visibility of the branch 11.x to hidden.

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

I am working on adding back the tests that were removed because they no longer applied in #91. I'm running into a problem where, in core/modules/content_translation/tests/src/Functional/ContentTranslationContextualLinksTest.php, an operation is tried with an admin user on admin/config/regional/content-language but even though $this->drupalLogin($this->rootUser); has been run, the HTML output states that the user has no access to the page.

I'm wondering whether the new Super user access policy could have something to do with this problem, but from what I've been able to deduce so far, the services.yml in use for these tests is the default.services.yml in sites/default and that has the policy set to true, meaning UID 1 should still be all-powerful. I would be grateful for assistance as this is currently blocking me from advancing with the tests.

RTBC +1.

Also checked at least that `path` does not appear anywhere in the templates as a variable. The only occurrence in menu templates of this theme hook refers to the twig function called path().

$ grep -r 'path' --include=menu-region--footer.html.twig
./core/modules/navigation/templates/menu-region--footer.html.twig:          attributes: create_attribute({ 'href': path('help.main'), 'data-drupal-tooltip': 'Help'|t, 'data-drupal-tooltip-class': 'admin-toolbar__tooltip' }),

kekkis → changed the visibility of the branch 3441615-navigationtheme-loads-extension.list.module to hidden.

An open and honest question in addition to the one in the IS: would we want to opt for auto-wiring instead of adding the create methods? It's the new in, hip and pop thing available in Drush 12.5. At least using create methods gives us a little more backwards compatibility.

Fixing Version info.

kekkis → created an issue.

HeikkiY → credited kekkis → .

Looks fine to me. I have reviewed the code quite thoroughly with Heikki.

kekkis → created an issue.

Added logging for the user and fixed coding style issues in the additional code part.

Actually I just noticed an issue with the patch. Unlike the entities that get saved, the log message does not identify the user who is doing the change. I will provide a new patch which does that.

I like the idea of a follow-up ticket and your idea about the select element which makes more sense than two distinct checkboxes. Still have not tested this anywhere though, I encourage other module users to test and report findings!

Nice patch. The watchdog/logger logging could also be a setting like in role_watchdog, see https://git.drupalcode.org/project/role_watchdog/-/blob/2.1.0/role_watch... for example.

I'm keeping this as NR for now but want to raise a question: should we add form validation to make sure that at most 1 consent mode has been enabled?

Confirming fix working on Drupal core 10.1.8, field_permissions 8.x-1.3.

Not sure if the module has tests (didn't check) or if the fix actually is logically correct, so leaving as NR instead of moving to RTBC.

I have to take back my takeback. In Drupal 10.1 and below, the field configuration form and the field storage configuration form were two separate forms. In Drupal 10.2, the forms have been combined into one. Changing the hook this way would break the module when used with Drupal 10.1 (or below). If I'm reading the code correctly, changing the hook this way (from using form id field_storage_config_edit_form to using form id field_config_edit_form) would make the alterations move from the field storage config form to the field config form which is an entirely different thing in D10.1 and below. So then basically the submit function added in the hook, ui_patterns_settings_form_field_storage_config_edit_form_submit, will run on the field config rather than field storage config, meaning that the stored configuration would change, and most probably would not work correctly in other parts of the module anymore.

Based on the above I'm setting this back to Needs work. A solution should be found that works both on Drupal 10.1 and Drupal 10.2, at the very least, based on the project page's statement of supported major versions.

Changing the hook from the one used in <=10.1 to one only present in 10.2+ is probably unwise, if the goal of this module is to support core versions other than latest stable. @Ahmad Abbad, can you specify in more detail how the deprecation causes the actual error? Deprecated methods and functions (and hooks) are just that, deprecations; they should not break anything but should work as reminders that a new implementation is needed in the longer run.

Thanks for the patch. The testing configuration is not up to date in this project, we should add a gitlab setup to get access to testing again.

Thanks for posting the issue! I happen to know of a solution that is already in use: https://github.com/Tampere/tampere-fi/blob/main/patches/media_entity_dre...

We could combine these approaches maybe?

Added core/once support and corrected the use of the data attribute in the MR. See https://api.jquery.com/data/ especially regarding the format of the data attribute name when reading it using jQuery.data().

@le72, if the patch worked for you, why did you decided to delete it from the issue?

Thank you for the contributions here, I assigned credit to everyone involved. Sorry that I didn't think to add this comment in the same status update.

RTBC +1. Created a local patch from MR in #28 and it fixes our problem which is exactly what the test describes.

kekkis → created an issue.

Added a small correction in the info file in commit e49f42f6.

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

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

The return type of \Drupal\migrate_plus\Plugin\migrate_plus\data_parser\Json::getSourceData cannot be set to array reliably since it might return NULL (per its own code) or any scalar (from json_decode). This is my suggestion how to fix it, even though using mixed might do as well.

Uploading new patch which is based on 2.1.x. Also providing interdiff, even if that now contains changes already present in the published version 2.0.0 since they weren't yet present in the patches here.

Please @hartsak if you are able, test the issue fork with a D10 installation to see whether my changes are valid.

Updating status and base version to rerun some testing and to create a MR.

I am choosing a different path with the versions. I created a different issue for pure D10 support (see https://git.drupalcode.org/project/media_entity_dreambroker/-/merge_requ... and #3395467 📌 Support Drupal 10 Active ). I will publish that version as 2.0.0 and will add a 2.1.x branch where I'll incorporate these changes. Thanks again for the efforts!

kekkis → created an issue.

Yes, and that is why I encourage you to publish a new release with that fix in place.

Upon further exploration I discovered that the query was what was wrong!

The original request, or its most relevant parts anyway, are here, split by ampersand:

&forceElevation=true
&enableElevation=true
&fl=%5Belevated%5D
&fl=ss_search_api_id%2Css_search_api_language%2Cscore%2Chash
&elevateIds=eob6nl-training_search-entity:node/2019:fi

Notice that neither of the generated fl parameters include the id field. When I added that in the query, Solr stopped complaining.

To make this work in the module, I basically forced the addition of the id field whenever either an excludeIds or elevateIds was being added.

And so as I was about to add a patch for this, I discovered commit f6896391 which already fixes the issue. So adding a related issue with this comment.

@sbrenner02, I'm basically running into exactly the same issue as you are. I wonder if @smustgrave could share his solrconfig*.xml files here for comparison. I'm not entirely sure the problem lies in those though, but I would like to see.

For reference, my config files are attached and the versions I'm running are:

Drupal core 10.1.4
Search API 8.x-1.29
Search API Solr 4.3.0
Search API Best Bets 2.0.2
Solr 8.11.2

Installed search_api related modules:

  search_api: 0
  search_api_autocomplete: 0
  search_api_best_bets: 0
  search_api_solr: 0
  search_api_solr_autocomplete: 0

I debugged the query so far that I got as far as a working and a non-working query. The difference between those is that the one that does not work has the elevateIds query parameter. An otherwise identical query does return results correctly, except of course with no elevation functionality. It does not matter what the value of the elevateIds parameter is: as long as the parameter is present, Solr returns HTTP 500 with the exact same stack as in @sbrenner02's comment. I'm adding that in as code for better readability:

java.lang.NullPointerException
  at org.apache.solr.response.transform.BaseEditorialTransformer.getKey(BaseEditorialTransformer.java:79)
  at org.apache.solr.response.transform.BaseEditorialTransformer.transform(BaseEditorialTransformer.java:54)
  at org.apache.solr.response.transform.DocTransformer.transform(DocTransformer.java:85)
  at org.apache.solr.response.transform.DocTransformers.transform(DocTransformers.java:76)
  at org.apache.solr.response.DocsStreamer.next(DocsStreamer.java:102)
  at org.apache.solr.response.DocsStreamer.next(DocsStreamer.java:59)
  at org.apache.solr.response.TextResponseWriter.writeDocuments(TextResponseWriter.java:194)
  at org.apache.solr.response.TextResponseWriter.writeVal(TextResponseWriter.java:137)
  at org.apache.solr.common.util.JsonTextWriter.writeNamedListAsMapWithDups(JsonTextWriter.java:387)
  at org.apache.solr.common.util.JsonTextWriter.writeNamedList(JsonTextWriter.java:293)
  at org.apache.solr.response.JSONWriter.writeResponse(JSONWriter.java:73)
  at org.apache.solr.response.JSONResponseWriter.write(JSONResponseWriter.java:66)
  at org.apache.solr.response.QueryResponseWriterUtil.writeQueryResponse(QueryResponseWriterUtil.java:65)
  at org.apache.solr.servlet.HttpSolrCall.writeResponse(HttpSolrCall.java:887)
  at org.apache.solr.servlet.HttpSolrCall.call(HttpSolrCall.java:580)
  at org.apache.solr.servlet.SolrDispatchFilter.doFilter(SolrDispatchFilter.java:427)
  at org.apache.solr.servlet.SolrDispatchFilter.doFilter(SolrDispatchFilter.java:357)
  at org.eclipse.jetty.servlet.FilterHolder.doFilter(FilterHolder.java:201)
  at org.eclipse.jetty.servlet.ServletHandler$Chain.doFilter(ServletHandler.java:1601)
  at org.eclipse.jetty.servlet.ServletHandler.doHandle(ServletHandler.java:548)
  at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:143)
  at org.eclipse.jetty.security.SecurityHandler.handle(SecurityHandler.java:600)
  at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:127)
  at org.eclipse.jetty.server.handler.ScopedHandler.nextHandle(ScopedHandler.java:235)
  at org.eclipse.jetty.server.session.SessionHandler.doHandle(SessionHandler.java:1624)
  at org.eclipse.jetty.server.handler.ScopedHandler.nextHandle(ScopedHandler.java:233)
  at org.eclipse.jetty.server.handler.ContextHandler.doHandle(ContextHandler.java:1434)
  at org.eclipse.jetty.server.handler.ScopedHandler.nextScope(ScopedHandler.java:188)
  at org.eclipse.jetty.servlet.ServletHandler.doScope(ServletHandler.java:501)
  at org.eclipse.jetty.server.session.SessionHandler.doScope(SessionHandler.java:1594)
  at org.eclipse.jetty.server.handler.ScopedHandler.nextScope(ScopedHandler.java:186)
  at org.eclipse.jetty.server.handler.ContextHandler.doScope(ContextHandler.java:1349)
  at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:141)
  at org.eclipse.jetty.server.handler.ContextHandlerCollection.handle(ContextHandlerCollection.java:191)
  at org.eclipse.jetty.server.handler.InetAccessHandler.handle(InetAccessHandler.java:177)
  at org.eclipse.jetty.server.handler.HandlerCollection.handle(HandlerCollection.java:146)
  at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:127)
  at org.eclipse.jetty.rewrite.handler.RewriteHandler.handle(RewriteHandler.java:322)
  at org.eclipse.jetty.server.handler.gzip.GzipHandler.handle(GzipHandler.java:763)
  at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:127)
  at org.eclipse.jetty.server.Server.handle(Server.java:516)
  at org.eclipse.jetty.server.HttpChannel.lambda$handle$1(HttpChannel.java:400)
  at org.eclipse.jetty.server.HttpChannel.dispatch(HttpChannel.java:645)
  at org.eclipse.jetty.server.HttpChannel.handle(HttpChannel.java:392)
  at org.eclipse.jetty.server.HttpConnection.onFillable(HttpConnection.java:277)
  at org.eclipse.jetty.io.AbstractConnection$ReadCallback.succeeded(AbstractConnection.java:311)
  at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:105)
  at org.eclipse.jetty.io.ChannelEndPoint$1.run(ChannelEndPoint.java:104)
  at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.runTask(EatWhatYouKill.java:338)
  at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.doProduce(EatWhatYouKill.java:315)
  at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.tryProduce(EatWhatYouKill.java:173)
  at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.run(EatWhatYouKill.java:131)
  at org.eclipse.jetty.util.thread.ReservedThreadExecutor$ReservedThread.run(ReservedThreadExecutor.java:409)
  at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:883)
  at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.run(QueuedThreadPool.java:1034)
  at java.base/java.lang.Thread.run(Unknown Source)

I think one way to fix this might be to add a hook_theme_suggestions_alter implementation in the module itself to add back the old suggestion. I'm not sure if I can think of other ways off the top of my head. As I mentioned I thought I was seeing an actual theme hook change but actually in https://git.drupalcode.org/project/group_content_menu/-/blob/8.x-1.x/gro... you will notice that hook_theme only defines group_content_menu and menu__group_menu still. So something is still a bit off maybe here in the original change too, I reckon?

Or if I am misunderstanding the whole original change then please inform me. :-) I'm just seeing that the theme hooks in use (using theme debugging via the HTML comments) are different now than they were in the previous 1.x release, without fallback to the old one.

I misinterpreted where the change was made, so I have no direct fix in mind for this. Maybe taking back the changes in the referenced issue and relying on hook_theme_suggestions_alter might be the most correct way. But if we can't have that (as I for example am a bit hesitant to propose such a change), shouldn't there at the very least be a change record for that original issue?

kekkis → created an issue. See original summary → .