@s_leu

s_leu → changed the visibility of the branch 3445462-3.0.5 to hidden.

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

s_leu → created an issue.

s_leu → changed the visibility of the branch 3440304-non-default-revision-revert-10.x to hidden.

After discussing this with amateescu, the logic for altering the revert operation access results shouldn't live in an override of NodeAccessControlHandler because this will clash with what the Trash module is doing. Instead, the coresponding code in core needs to change, I filed an MR for this in 🐛 Revision revert links in workspace respond with 404 not found Active .

The MR i opened here in combination with the changes of the MR for 📌 Allow viewing and reverting to live revisions inside workspaces Active will allow reverting to non default revisions inside a workspace. It's worth to note that the MR for wse will also enable listing of live revisions inside workspaces.

The MR here will only change core's entity and node access control handlers in a way which will allow us to alter access for revert operations on default revisions.

s_leu → changed the visibility of the branch 3440304-revision-revert-links to hidden.

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

s_leu → created an issue.

@heddn Thanks for reviewing again. It seems I missed some code in my latest push before that comment. I pushed further changes for this now.

Some feedback on your latest comment above:

• I cannot see an item in live listed as a suggestion after doing a manual translation of corresponding menu link via translate op on the menu tree edit form
• I think generally speaking, items which already have a translation in the target language and target workspace shouldn’t be listed in the suggestions in the first place. Should I file a follow up for this?
• Could you please test latest changes in the MR and if it doesn’t work, please provide step by step test instructions to replicate the problem?

@smustgrave

Wonder if it would be easier to do the screenshot of setting the width/height to the drupal-media tag.

Of course my view on this is a bit biased, but using drag and drop handles as in the native ckeditor5 image plugin for resizing seems much more editor friendly to me.

This is also true if comparing the drag and drop resize to the current state i saw in the Drupal CMS media track where you have like a dozen image styles to select from. If the goal is to make things easier for people that don't know Drupal, then selecting image styles is the wrong path to take IMO.

The module has a few open bugs but I experienced a few times the resizing would break things.

Did you report the bugs in the issue queue of the module?

If I remember correctly the resize creates dynamic image styles?

No. A resize has a width and based on that width, the image is then rendered in one of the avaialble (and for now not configurable) image styles based on the width. I.e. if the resize is 150px wide, an image size with a small width up to 300px (no upscaling) is used. If it's 500px the image style with max width of 650px is used (again, no upscaling).

The case for editors to have control is clear but the case why some people might not want this feature might be less than clear, such as:

In case of the MR I posted above, admins can simply disable the resize filter on the text format and resizing won't be applied. So different formats could be used to control this as each format has a corresponding permission as well.

This is working but the messages are only added to the job once its translation comes back/gets accepted.

I wonder if we should write an update hook or even a small form or hook_cron that allows scanning for such jobs (which are pointing to irrelevant workspaces), lists and updates them. That would allow a more immediate feedback on such jobs to users.

If we wanted to improve this situation a bit more, we could implement hook_workspace_update() and possible hook_workspace_delete() in this module to display/log warnings referring to the jobs and also updating the current jobs adding the message that is added in the MR.

s_leu → changed the visibility of the branch 3490853-revisions-of-nested-ws to hidden.

Pushed an initial port of the code from ckeditor_media_resize → . This will definitely need some more work (at least cleaning up spaces) but it would be good to get some initial feedback on it before proceeding further, so setting NR to get some feedback.

There revert button for the latest live revision isn't accessible inside workspaces and CI fails.

This is ready for a first review.

Thanks for the review, I addressed the feedback.

Thanks for the review! I addressed the feedback adding a conflict message with links to conflicting workspaces as well as a visual indicator that makes it clearer that the row has a conflict in comparison with rows of items not having conflicts.

s_leu → created an issue.

s_leu → created an issue.

The MR looks good to me. One consideration here: As the presave hook is so expensive, it may be worth triggering a hook/an event that allows other modules to react when the hook is fired to skip further execution?

s_leu → created an issue.

Created an MR that addressed this issue, also updated IS, steps to reproduce and title.

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

++ for the patch in #26, please get this committed.

s_leu → created an issue.

Chi is right and I pushed the corresponding changes into the MR.

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

The patch works as expected.

s_leu → changed the visibility of the branch 2546212-entity-viewform-mode-10.3.x to hidden.

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

I recently looked at a way on how to expose access denied messages a bit better instead of simply showing the "Oops" message and found this issue here.

Looking at the MR i think it's a good start and I had some ideas on how to improve this a bit further. Here some proposals:

1. we can actually make this a) more useful for developers (if error logging is set to verbose) by putting whatever is contained inside xmlhttp.responseText into the message instead of the hardcoded "Oops" or the configurable message from the MR above. That would be a nice DX improvement, it'd enable instant view of backtraces and error messages in the message instead of having to look that up in the console after an ajax error. This should probably only be happening when $config['system.logging']['error_level'] is set to verbose
2. As for cases where the "Oops" message is shown after an access denied exception inside an ajax call, we could possibly display access denied reasons based on the reason specified in an instance of AccessResultReasonInterface. This should probably be configurable as well.
3. Whenever an ajax error message is displayed it would probably make sense to scroll the window to the message as otherwise it's easy to miss on pages with a height that's higher than the viewport.

Adding a PoC patch that addresses the first two points here for reference. It's definitely not ready for production but could possibly serve as a base for making further improvements before merging the MR here. Note the todos and also that the patch is against 10.3.x.

Thanks for writing up the issue @heddn. I can confirm the changes are resolving the fatal errors, but I guess we should wait for others before marking it as RTBC.

heddn → credited s_leu → .

Can confirm that #10 is still applying and working for us too. As 3 people confirmed that this is working and the patch contains tests, I'm taking the liberty of marking this as RTBC.

alexpott → credited s_leu → .

s_leu → created an issue.

The $entity->delete() revealed that we're missing to install the users_data table, this failed in a custom test that was extended GroupKernelTestBase, so adding an install_schema() call to fix that.

Also there's a hidden branch with changes that combine these here with the ones for 🐛 Node update hook is triggered upon group content create Needs work comment #54 🐛 Node update hook is triggered upon group content create Needs work in case someone needs this too.

s_leu → changed the visibility of the branch 2754399_2872697_2.x_combined to hidden.

s_leu → changed the visibility of the branch 3278936-batched-group-delete-2.2.2 to hidden.

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

s_leu → changed the visibility of the branch 2754399_2872697_2.x_combined to hidden.

s_leu → changed the visibility of the branch 2754399_2872697_2.x_combined to active.

s_leu → changed the visibility of the branch 2754399_2872697_2.x_combined to hidden.

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

s_leu → changed the visibility of the branch 3391226_3371633_3471130_combined to hidden.

Initial MR is ready for review/testing

s_leu → created an issue.

I tried the hook_editor_js_settings_alter() example and it works well for me.

Trying this with a file that's approximately 14GB large, another issues surfaced. The filesize column is of type int and may be too small for really large files, see attached screenshot. I pushed a change containing an update hook which will change the filesize basefield into a big bigint DB column to tackle this.

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

amateescu → credited s_leu → .

s_leu → created an issue.

Re #38 : I don't think that issue solved the problem with the pager here and it's neither addressing the issue of sorting which is addressed here.

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

+1 for this feature. It's a WYSIWYG editor after all, so at least CSS should be rendered when previewing.

The image style is exposed to config with the code from the MR. The part with the download of the alternative icons is yet missing, but I think this can be reviewed as is and the rest added later.

s_leu → created an issue.

Diff will move to support all entity types generically.

Please correct me if I'm wrong, but I think in order to support groups there will still be a requirement for some extra code, at least in order to consider group permissions when rendering the view/revert/delete links on the revision overview form. I just added this case in the MR.

Agree that this logic should probably live in a separate module, possibly a submodule of diff that depends on group? But as you say, until the "generic entity support" refactoring for the diff module happened, it probably doesn't make sense to proceed any further.

Re-rolled the patch from #8 on top of 2.x into an MR, fixed a bunch of issues reported by PHPCS and addressed review comments in #10. phpstan fails are still tbd, but moving into review anyway as it would be good if someone would test and review the code changes.

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

entity.group.version_history now shows in admin theme

This seems a to work only randomly depending on the apparently random order of route subscribers, which in you case seemed to be correct. The order seems to be different in my case and to ensure that the admin theme is used, we need the altering in GroupRevisionRouteSubscriber to run before the one in GroupAdminRouteSubscriber. A corresponding change can be found in the MR.

Btw. I wasn't sure which target branch to use, please change it if it's incorrect.

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

The patch from #29 didn't apply any more on the latest version, so I created an issue fork and PR that fixes that.

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

s_leu → changed the visibility of the branch 3392621-cke-media-preview-post to hidden.

s_leu → created an issue.

I think we should take a much simpler approach and just check if the Workspaces module is enabled, and allow alias generation if a workspace is active.

That's more or less what I had in the beginning of the MR.

Ok I'm not sure what's going on here in the Validatable config step of the merge request pipeline.

The code in the MR adds a post update hook which installs a view mode config and apparently that differs to the output of the drush config:inspect --statistics command which is ran after installing the standard profile first and then again when checking out the branch and running update hooks. So at this point of course config will differ as the view mode got installed. What I don't understand here is why this would be a reason to stop the pipeline? I mean it's legitimate (and sometimes necessary) for a module to install new config in an update hook, isn't it?

I also tried to add that config to the optional config of the standard profile, but the error causing the fail persisted. Can someone point out why this happens and what we need to do to fix this? This didn't happen a while ago when the merge request pipeline was green..

s_leu → created an issue.

s_leu → changed the visibility of the branch 3392621-cke-media-preview-post-10.2.x to hidden.

Fixed the issue with the note appearing contextual links as mentioned in #325 and #328. To those people who still upload patches in this massive issue: Nobody will see what you changed if you don't at least upload an interdiff along with your patch. But even if you do and there's an MR in the issue, please just use the MR to push your changes, that will make collaboration much easier.

Thanks for the quick review, merged.

At some point we might also want to drop the \Drupal\wse_scheduler\Event\WorkspaceSchedulerEvents class and use the event class name. I didn't want to do this yet as it might breaking changes on some sites that subscribe to the event.

s_leu → created an issue.

Hey smustgrave, I know 20'000 is a lot but this is guaranteed to break things. The point is mainly that we shouldn't send something that could possibly a long string inside a GET parameter. This could already break on way less than 20'000 characters on some servers. See for example 💬 Extremely long Views AJAX query string triggers 403 in AWS Postponed: needs info .

Adding a patch that still applies on 10.1.x as the latest patch here doesn't any more.

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

s_leu → created an issue.