- 🇬🇧United Kingdom catch
Looks good. Committed/pushed to 11.x, 11.1.x and 10.5.x, thanks!
- 🇳🇱Netherlands bbrala Netherlands
Made selection code better and added comments on how it works.
Getting a failure on the Configuration tests. But not sure why, perhaps because the fixtures need to change since default config changed for taxonomy config?
- 🇬🇧United Kingdom catch
Re-titling because I keep thinking this is about the governance issue and security review, which are more or less done at this point, whereas we still need to actually add the dependency to core. Would be good to get an MR up to add it.
- First commit to issue fork.
- 🇺🇸United States smustgrave
So should this be closed out? Want to give it one more change before closing out. If no follow up can be closed in 3 months.
- 🇬🇧United Kingdom aaron.ferris
This will need work, although certain aspects of the patch will still apply others won't - please raise an MR. (Appreciate its old!)
- 🇺🇸United States smustgrave
Thanks summary reads well
I was able to replicate following the steps and the MR does appear to address the problem
Test coverage seems to be there too.LGTM
- 🇳🇱Netherlands bbrala Netherlands
Now running on onSave in the updateAll method.
Also added an extra check for when we have no reference from system.rss and there is an available view_mode called default.
- 🇳🇱Netherlands bbrala Netherlands
Still need to handle 2 things:
- When default is the viewmode, and we have no config this will keep updateing to the first key. Oops.
- Run this noSave
- 🇬🇧United Kingdom catch
Just revisited this issue based on 📌 Require password confirmation to install new code Active and I think we might want to split the UI into its own issue - if we use development mode to suppress password re-authentication, we either need to not have a UI, or put the development mode settings themselves behind password re-authentication.
- 🇬🇧United Kingdom catch
🌱 [Meta] Move module.theme.css files to Classy or Seven Postponed: needs info is open from the child issues, and was rescoped to Claro.
It's not clear to me if the child issues represent all the intended work here or not. If they do, we've got one or two left and it could be closed. If it doesn't, then the entire issue summary needs a rewrite with clear next steps.
- 🇺🇸United States smustgrave
Since both themes have been removed should this be closed.
- 🇺🇸United States smustgrave
Came up as a daily BSI target.
1 is this still an issue in D11
Will need a complete issue summary to continueIf no follow up could be closed in 3+ months
- 🇺🇸United States smustgrave
Since there hasn't been a follow up going to close out.
- 🇷🇴Romania amateescu
@adamzimmermann
It feels weird to need a contrib module for core functionality to work though.
That's because core moves at a slower pace, and production sites needed a lot of various fixes for Workspaces that were lingering for years in the core queue for various reasons. Workspaces Extra had to incorporate all these little fixes to prevent developers from using a huge list of patches into their projects.
Opened a MR with a fix and test coverage for this issue.
- 🇳🇱Netherlands bbrala Netherlands
I've got some questions about your approach, there might be more gotcha's (and possibly a bug :P)
- @amateescu opened merge request.
- First commit to issue fork.
- 🇳🇱Netherlands undersound3
We are also experiencing issues with this patch https://www.drupal.org/files/issues/2024-12-17/2822764-129.patch →
InvalidArgumentException: Invalid translation language (und) specified. in Drupal\Core\Entity\ContentEntityBase->addTranslation() (line 985
When using 3.x dev version with patch https://www.drupal.org/files/issues/2024-02-23/inline_entity_form-282276... →
things work as intended....at least for our setup. Entity reference field on a node and translations disabled - 🇬🇧United Kingdom longwave UK
Responded to @catch's review, NW for these changes.
- 🇬🇧United Kingdom catch
Left a couple of (reasonably horrible) questions on the MR.
- 🇳🇱Netherlands bbrala Netherlands
Well, seems i have a working test. While going through the motions i did find a one more view that use default still.
views.view.taxonomy_term.yml
row: type: node_rss options: relationship: none view_mode: default
Also a little confused. If i look at
system.rss.yml
the default should berss
i think, since that was in the config. But when running the update test it gets set totitle
, i really don't understand how that is possible. What am i missing there? - 🇺🇸United States smustgrave
Just following up if anyone has been able to verify #11, if no follow up could close out in 3 months.
- 🇺🇸United States smustgrave
Just following up if valid request still? If no follow up could be closed out in 3 months.
- leymannx Berlin
True, now the hook is in src/Hook/ContentTranslationHooks.php?ref_type=tags#L382-415 and the 11.x MR still has it, while the 10.x patch has the hook removed.
But all tests are green 🤔
Are the tests wrong? Or could the hook still stay?
- 🇳🇱Netherlands Lendude Amsterdam
Opened 🐛 Add ViewsConfigUpdater deprecation support for update_table_css_class Active to address this lack of deprecation, also it's not being run from updateAll, so new module config is not getting updated either it looks like
- 🇬🇧United Kingdom catch
The post update / ViewsConfigUpdater method added here doesn't handle deprecations.
views_post_update_update_remember_role_empty
andviews_post_update_views_data_argument_plugin_id
both do implement it so could be used for examples.🐛 Add proper deprecation notices in config entity presave bc layers Active has more details. We can open a follow-up to implement that, or revert this and add it back with support, but one of those should happen before 11.2.0 so re-opening for now.
- 🇩🇪Germany a.dmitriiev
MR doesn't have all the changes from patch #58. Now the hook `content_translation_language_fallback_candidates_entity_view_alter` is in the class, so MR actually does not remove it, but patch - does.
- 🇳🇱Netherlands bbrala Netherlands
Little more context; https://drupal.slack.com/archives/C079NQPQUEN/p1744791038514039
- 🇬🇧United Kingdom longwave UK
Any existing views that have
default
as the RSS view mode need to be updated so they store whatever the default value actually was, before we delete that value from config. - 🇳🇱Netherlands bbrala Netherlands
Added a change record.
All tests are green. Which is great.
Not sure about the upgrade path. I'll ask around
- 🇳🇱Netherlands bbrala Netherlands
Pushed to a MR with some small changes.
Upgrade path is tested afaik, so removing that tag. Has tests it seems, so removing that tag also. Did small update to IS, but don't see how we can make this more clear.
This will need a CR though. Dont think we need to deprecate anything really, since this is pretty much broken functionality you cant rely on.
- @bbrala opened merge request.
- First commit to issue fork.
- 🇺🇸United States rraney
I have some form of this issue on Windows Server with Drupal 10 and Bootstrap Barrio theme. I wasn't having issues with 777. The errors have to do with the "system" not being able to make a directory in the sites/default/files/php/twig folder and also renaming a file. The patch works. I'm curious if a remedy will be made in SDC, Bootstrap theme or core.
- 🇬🇧United Kingdom catch
Added a kernel test, which in turn uncovered a bug - pushed a commit for both.
The database query reduction now looks like 263 -> 247 when this issue is combined with the others.
I also found 🐛 Config overrides are loaded for English even when translate_english is false Active while looking at what queries were left, not really related to here except that it's a lot of config entity loads for no reason. 📌 ContentTranslationHooks::viewsDataAlter() could multiple load config entities Active is also over a dozen queries we can potentially consolidate.
- 🇦🇺Australia geoffreyr
Given that Drupal 7 has now reached its End Of Life → , this issue should be closed.
Thanks for all your hard work on this one. @joshmiller's patch in #28 is probably the approach you want to take to resolve this for your D7 sites, and can probably be managed within your codebases via Composer or Drush Patchfile, but it won't be merged into the Drupal 7 codebase.
This issue no longer applies in Drupal 8+ due to\Drupal\Core\Utility\Token::scan()
being strongly typed. - 🇺🇸United States j_s
It could be useful to potentially have both the php and htaccess fixes.
- furo Italy
These screenshots were taken in the /contact page of the Olivero theme
required message field:
required subject field:
- 🇺🇸United States smustgrave
Just following up if additional steps can be provided, else we could probably close this one out.
- @hswong3i opened merge request.
- @hswong3i opened merge request.
- 🇦🇺Australia acbramley
- First commit to issue fork.
- 🇬🇧United Kingdom catch
Updated the issue summary.
I combined this locally with the branch on 📌 Try to replace path alias preloading with lazy generation Active and together, they're knocking 40 database queries and 17 cache sets off the front page with a cold cache.
1) Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryFrontPagePerformanceTest::testFrontPagePerformance Failed asserting that two arrays are identical. --- Expected +++ Actual @@ @@ Array &0 [ - 'QueryCount' => 376, + 'QueryCount' => 336, 'CacheGetCount' => 471, - 'CacheSetCount' => 467, + 'CacheSetCount' => 440, 'CacheDeleteCount' => 0, 'CacheTagLookupQueryCount' => 49, 'CacheTagInvalidationCount' => 0,