🇷🇺Russia @ilya.no

I've only added new test, because I haven't found any existing test for core/lib/Drupal/Core/Config/ConfigInstaller.php.
Concerning changing of existing tests, I find comments #47 🐛 Process translation config files for custom modules Needs work and #49 🐛 Process translation config files for custom modules Needs work explanatory, but maybe I haven't delved enough into them.

I have the same problem. I've created a MR in order to fix the issue. Logic of my update is following - if there is an indicator, that options are submitted, but options themselves are absent from query param, then return empty array in order to avoid hiding all fields.

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

Sending issue back to review, as I've added changes for MR's comments and pipeline is green.

Attaching reroll for the latest version.

I have reproduced issue too. Need to create page manager page and add module's block. Then need to update any value and try to import config. Added comment in corresponding issue as well - https://www.drupal.org/project/social_media_links/issues/3023172#comment... 🐛 Missing config schema for block Needs work .

I've been using patch, made from the MR, via https://git.drupalcode.org/project/social_media_links/-/merge_requests/1..., but after upgrading to D10.3.2 I've got error, as described in https://www.drupal.org/project/social_media_links/issues/3461790 🐛 Schema error after updating to drupal 10.3 Closed: cannot reproduce .

I haven't traced the issue, but it seems to be related to the solution with src/Plugin/Config/Schema/Platforms.php, because when I removed corresponding changes and added code from #12 patch, then the issue was gone.

I'm attaching a bit updated patch with several fixes in order to have ability for deploy.

I think, that issue's priority should be set to Critical, because currently it's not possible to import configuration.
In order to reproduce, need to create page manager variant and add "Social Media Links" block.

Attaching patch with additional option added to schema.

I've changed `plugins_installed_version` to `plugins_version_installed`, which is actually used.
Also I've fixed points 1 and 2 from previous comment, but I haven't found any third party settings, provided by this module.
I mean, that currently it provides only settings configuration, which needs schema. Please, correct me, if I'm wrong.

Restoring previous status, as MR is rebased and pipeline is green.

As I stated in my previous comment #48 🐛 "Drupal\migrate_tools\Commands\MigrateToolsCommands" not found PathRedirectImportCommands Postponed: needs info I've created issue for Drush, because I haven't found solution from the Drupal side. Corresponding changes are merged in 13.x branch of Drush, so I'm not sure, when it will be available. But if this patch applies to your version of Drush, you can use it. My patch allows drush commands to be executed without fatal error and it allows deploy to be run fine.

I've fixed tests errors, so pipeline for the MR is fine now.

Initial patch didn't work for me, so attaching the patch, which fixed the issue.
My issue is following:
1) Drupal 9.5.11, webform module 6.1.5, default download method is private
2) There is webform, which is accessible by anonymous users and contains upload field of type `Document file`.
3) When admin tries to update submission via edit page `admin/structure/webform/manage/form_name/submission/1/edit`, there is same error, as mentioned in title.
Not sure, if my fix is proper one, but it seems logical to check `download` operation instead of `view` for files, which can be downloaded, but can't be viewed.

I've added one more missing schema definition.

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

I've added draft CR https://www.drupal.org/node/3449092 → and rebased MR.

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

I have this error, when site on D9 doesn't have migrate modules enabled, so when I update to D10 and this module to the latest version, then there is fatal error on evert drush command right after composer update.

In order to fix it from drush side, I've added this issue https://github.com/drush-ops/drush/issues/5860 and related PR, which can be used as a patch for composer https://patch-diff.githubusercontent.com/raw/drush-ops/drush/pull/5861.diff

Attaching initial patch.

ilya.no → created an issue.

As I understand no, because currently different mappings in schema contain plugin_id in their own place. In this issue we remove plugin_id from all these mappings and add it to the `views_handler` type at the first place.

Basically we don't need to reorder it for existing views, but we need to update default views from core modules, because otherwise tests fail. It happens, because test compares default config with actual and fails, as they are not the same.

I've pushed phpcs fix into MR, not sure, that I need to update patch for it.

I've created MR with changes from latest patch for further developing.

While checking failed tests for my patch, I've discovered following points:
1) My solution doesn't work for latest versions of Drupal, because it adds more mess into config schema definition and it affects views more heavy, than I've expected, so I've focused on another solution of that issue.
2) I've put a bit wrong description on `Steps to reproduce` section. If we create field and then enable aggregation, than issue is not reproduced. But if we create view, enable aggregation, add field and set its aggregation to COUNT, than issue can be reproduced with latest version of Drupal. So we first need to enable aggregation and only then add field and set aggregation for it.

I've fixed `Steps to reproduce` section and also removed my solution from `Proposed resolution` section. I'm adding new patch, which is rebase of the patch from 33 comment. I couldn't generate interdiff, but basically it's rebase for D11 with couple of fixes. I've also added hook_post_update. I haven't run all tests, but the ones, which were failing are now good, at least locally.

ilya.no → created an issue.

I've created MR and added typehints. I'm not sure about CR, because I have no feedback about my suggestion so far.

I've updated code, so there is no binary data anymore.

I've added test, but it's not shown in https://git.drupalcode.org/project/drupal/-/merge_requests/4926/diffs section. I guess, it's due to serialized lines.
Also I've found, that function getEntity() is called mainly when we create/edit inline block via UI. But there is another place, when unserialize() function is called and it's saveBlockContent(), which is called on presave hook. So I've added separate function in order to avoid code duplication. In test I create node where component in field layout_builder__layout field, which contains serialized data.

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

I've added typehint return to newly added functions. Also tests are ok now.

ilya.no → created an issue.

I've found, that my new test module's name is already occupied, so I've renamed and relocated it from locale to config_translation module.

I've fixed remaining comments, except for note about new class, because I'm not sure, whether this is in scope of this task.
I've removed LB related code from field_ui and recreated it as per first option of this note.
I've also fixed test and locally it passes fine, but on pipeline there is an error.

Thanks for feedback, but I have schema issues again. Attaching new patch and updating issue.

ilya.no → created an issue.

Attaching patch with test. Not sure, what is the best place for it, so I've chosen locale module, because I haven't found tests for `ConfigInstaller`.

ilya.no → created an issue.

I'm not sure, whether this issue is related to my problem, but in custom theme I have dependency on `jquery_ui_selectmenu/selectmenu` and it causes the error, saying, that library `is not defined because the defining extension is not installed. Cannot decide if it is deprecated or not`. The problem is that module `jquery_ui_selectmenu` is installed, but it doesn't have any libraries, because its library is added by `jquery_ui` module in `library_info` hook.

Attaching the patch, which fixes this issue.

Attaching patch with tests, which are taken from #20 comment. I've also updated issue summary with 2 possible solutions.

I've been pointed by @sorlov to the issue with duplicating title for date only case, so attaching another patch with the fix for datetime field widget. It adds logic to use title_display setting from the element, if there is no such setting already.
I've updated issue summary, but for the change record, I think, that we need to agree on one of ours solutions first and then add CR if necessary.

Problem is that if we use date fields on custom form, then they don't have titles with current patch. For testing I've copied FormTestLabelForm into custom module and saw, that only `date` form element has title. That's why test fails.

My solution is to use `form_element` as theme wrapper instead of `datetime_wrapper`, because we need usual label and input HTML structure. In my patch I remove occurencies of `datetime_wrapper`. Failed test runs fine locally after my update. I've also checked all 4 cases from #11 comment on node add form and titles are fine.

Changes look good, thanks!

Attaching the patch which fixes all 4 cases for me. I test with custom module, which has settings config in install folder and view config in optional folder. For both cases there are translations in language/fr folder.
Also updated summary.

I'm not sure in what way should we update summary, because for me it looks clear. Should we just add text from comment #24 🐛 Process translation config files for custom modules Needs work ?

I'm attaching draft patch, which fixes the case, when we install module on already existing site. So current patch fixes 3/4 cases. Need to fix the case, when module is enabled during site installation.

I've noticed, that ConfigInstaller manages all available collections, when installs default config, but when it comes to optional config, then only default collection is used. So, I've added code to use all available collections.

@catch RTBC is for patch #40.
Patch from comment #49 is for 10.0.x branch without any explanation and I think, that we can ignore it.

Fixing #17 patch with field storage settings addition.

I've noticed, that when we see list of libraries inside iframe in field widget, there is no icon for each library, located in web/libraries folder, because module tries to load it from sites/default folder.
So, here is the patch with the fix for this and also I've included latest changes from MR.

Updating patch in order to avoid error, if there is no answer on page reload.

Pushed initial update.

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

@murilohp hello! You can make your hook to be executed after views_infinite_scroll_preprocess_views_infinite_scroll_pager by increasing your module's weight. This can be done in several ways, described here → .

Fixing wrong function call.

Attaching patch with the fix for tests.

Sorry for the a bit wrong patch. Attaching proper one.

Attaching patch with new test case.
About &title= part, I've tested following case - I updated 'test_content_ajax' view and switched off AJAX option and checked URLs and this part was presented. So, I assume this as normal behaviour, as non-AJAX view has the same URL.

Thanks for the fix. I've added fix for tests.

Attaching patch for the latest version. I haven't succeeded with tests so far, will get back to them later.

@smustgrave I'm not sure, that I've got your point. In comment #10 it's stated, that we need to add test case, which looks like

assertFalse($password->check($a_password, NULL))

In my patch I've added following code

$this->assertFalse($this->passwordHasher->check($this->password, NULL));

which looks the same, as suggestion from comment #10.

Could you correct me, if I'm wrong? Or maybe I didn't understand you. Thanks!

ilya.no → created an issue.

Hello @benjifisher. I'm attaching draft patch for your suggestions from comment #250. It almost works with 2 points:
1) Field is added, but it doesn't look, that it's fully functional in terms of content editing.
2) Route for configuration page /admin/config is hidden even if user has items in it.
So it's incomplete, but I need to know, how it's close to your thoughts.

In MR, attached to this issue, I've added new option to `admin/config/content/migrate-generator-export` page. If we choose `No column, directly save file along with result csv.`, another field should appear - `Folder, where file should be saved to.`. In this option we should point, where files should be saved. We need this option on settings level, because file files are processed via separate processor plugin, which uses this option. I've tested locally and files are copied fine. In result csv file, in column for this field, there full paths to just copied files.

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

Hello @lostcarpark and @markie
1) https://www.drupal.org/project/smart_trim/issues/3368700 📌 Latest version's smart_trim_update_10201() function doesn't work with layout_builder Needs work
2) https://www.drupal.org/project/smart_trim/issues/3368694 🐛 Remove token_browser from saved configuration of formatter Fixed
For second one I've created the patch, since it requires less work. For the first one, will try to work on it later.

ilya.no → created an issue.

ilya.no → created an issue.

I've discovered issue with new logic. Attaching updated patch and interdiff.

ilya.no → created an issue.

ilya.no → created an issue.

ilya.no → created an issue.

Thanks for the improvement! I have couple of issues:
1) When entity view display uses layout builder, than smart_trim_update_10201 function doesn't work, so need to manually re-save all such configs.
2) When saving config and then export it, there is `token_browser: ''` added to `more` section of config. This results in `missing schema` error from Configuration Inspector → module. So have to remove it manually and import config again.

ilya.no → created an issue.

About first point of #221 comment, order of these elements are set directly in formatter, while in other places order is opposite. So I agree, that it's not possible to easily fix it.
About second point I've made a patch, where simply set open state for `formatter` and `settings` form elements. Besides that I've modified logic to include case, when view display uses layout_builder.

Thanks for the patch! In addition, I had another error 'TypeError: Drupal\externalauth\Authmap::getAuthData(): Argument #1 ($uid) must be of type int, string given, called in /ldap/ldap_authentication/src/Controller/LoginValidatorLoginForm.php on line 201 in Drupal\externalauth\Authmap->getAuthData() (line 74 of /externalauth/src/Authmap.php)'
It happens, because $account->id() returns user ID as string.
I'm attaching patch with this error fixed, so password update completely works for non-ldap user.
For comment #9 I'm not sure I got the points, as I don't see error in logic, because we set $this->authName only, if we receive correct data from ldap, so it's clear to me, that we should check this before we try to use it.

I think, that we can ignore the patch from comment #25, because it adds changes from #3227667 → issue, so one can use one patch for implementing both features.
Attaching interdiff.

@larowlan I agree with your suggestion and here is the patch for your comment #10. I've found, that PasswordHashingTest is now LegacyPasswordHashingTest due to recent changes in password processing logic. So, I've made a change to the new service and to the old one, because as long as this code is presented it needs to work properly although it's deprecated. Please, correct me, if I'm wrong.

Attaching patch for contrib module.

Attaching patch for contrib module.

I've tested on D10 site and haven't reproduced issue neither. I think, that patch from comment #51 🐛 Aggregated entity fields cause SchemaIncompleteException Needs review is enough for sites, using D9, while issue itself may be closed.

Attaching patch for new version of module and interdiff, because initial patch doesn't help when using 1.3 version of this module.
Concerning above feedback, currently layout_builder module describes layout settings as layout_plugin.settings.[%parent.layout_id] and core doesn't provide any schema for additional property in layout_plugin.settings, so we have to make such override to cover all possible layouts.

I've fixed tests and some comments. For other comment I've written answers, because they are not fully clear to me.

@andypost I've removed unnecessary changes, added hook update and added notes on MR. I'm not sure about tests, because, as I see, currently there is very basic test, which checks, that site is not crashed after module is enabled. So, before writing tests for new field, we need to write tests for entities operations and I'm not sure it's in scope of this task.

I've created MR with this feature. Initially I tried to use hook_block_access in toc_js_per_node.module file, but it doesn't work for node pages, rendered with layout builder → , so I added function blockAccess and it works fine.

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

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

Attaching the patch, please, have a look.

Attaching patch for 9.5.x-dev version. Works locally. Tried to fix tests, but no luck so far. Interdiff may be not correct, since some test files were moved to hal module.

I've checked `/admin` page with blackfire on Drupal 9.3.7, site is built within skilld docker container.

Without patch:

Time 211 ms
I/O Wait 2.42 ms
CPU 209 ms
Peak Memory 18.4 MB

With patch:

Time 234 ms
I/O Wait 1.2 ms
CPU 233 ms
Peak Memory 19 MB

Result
Time +22.9 ms (+10.8%)
CPU +24.1 ms (+11.5%)
Peak Memory +583 kB (+3.17%)

Attaching some screenshots with comparison.
Please, let me know, if this is ok, or maybe I need to provide more comprehensive data.

Attaching patch with fixes for 1 -3 points. I haven't proceeded with profiling yet.

Attaching updated patch. For some reason patch from #218 🐛 Restrict access to empty top level administration pages Fixed changed important line without any explanation why. I've brought initial logic back, because following code is senseless, when we have several roles:

$route->setRequirement('_role', (string) implode('', $route_accessible_roles));

We need to append them with `+` sign in order to indicate, that user of any of these roles should have access to this link.

$route->setRequirement('_role', (string) implode('+', $route_accessible_roles));

I've also added rebuilding routes command to tests and group option to config route item. Change for cron access was removed, because it seems to be out of scope, please, correct me, if I'm wrong.

Hello, Paulo! Thanks for the review. Yes, need to write some tests.