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.
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.
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.
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.
I've added typehint return to newly added functions. Also tests are ok now.
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.
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`.
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.
@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!
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.
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.
I've discovered issue with new logic. Attaching updated patch and interdiff.
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.
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.
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.
I've tested again and it works ok for me. The only problem I had is contrib module Menu Admin per Menu → , which adds its own custom access checker for menu 'entity.menu.collection' and return positive access result. But when I disable this module, logic works fine. Added couple improvements for tests.
Attaching patch for D9 with interdiff for previous patch. I've fixed adding roles with OR conditions, instead of previous AND. Also added code for checking menu tree for separate function, because in admin/config menu tree we have other routes with possible empty content, so now it's recursive function.
I'm currently using latest patch and noticed, that new options have been added to the configuration, but no corresponding schema. Attaching patch with added schema changes.
Here is what I suggest:
1) Add new option to routes, called for example '_group_route' and set it to TRUE for necessary routes.
2) Implement new route subscriber, which is executed during 'onAlterRoutes' event.
The idea of this subscriber is following: for each route with '_group_route' set to TRUE we check, if it has accessible children by any roles with 'access administration pages' permission. If we find such links, we set requirement '_role' witch points which roles should have access to this route. If there is no such links, we add '_roles' anyway with only admin role.
Attaching patch, it doesn't have any tests so far, because I think, at first we need to check, if this idea is acceptable.