🇪🇸Spain @rodrigoaguilera

Can you try the MR from 🐛 Issue upgrading from 8.x-1.4 to 8.x-1.10 Active
It includes fixes for the warning reported here also

Thanks for reporting. I added checks around the code that is giving warning including the ones from the other issue linked.
Can you try the MR?

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

Please review

With the change from 🐛 Move hook_entity_delete to hook_entity_predete Active and the MR I proposed the error is gone for me.

Please review the redirect issues first as it would be a requirement to for the fix here.

rodrigoaguilera → created an issue.

I'm facing the error from the summary with the latest version of this module and redirect 8.x-1.11 so I think it is still happening.
The relationship gets deleted and then the redirect module can't build a canonical URL for deleting redirects.

I think it makes sense for this module to perform its action on the predelete and I think redirect should do too.
I'm going to propose that change

Please review

rodrigoaguilera → created an issue.

Please review

rodrigoaguilera → created an issue.

I don't maintain any projects that require this anymore so it can be closed from my point of view

Please review

I have the same issue but on Drupal 10 and I can't upgrade yet.
I attach a patch for Drupal 10.4.8 in case it can help anyone

rodrigoaguilera → created an issue.

This would be a change in one of the files of the repository, reame.md, so anyone can propose that change with a merge request.

https://www.drupal.org/community/contributor-guide/task/create-a-merge-r... →

Once the merge request is merged I can edit the description of the module page and use that readme markdown file to transform it into HTML.

I faced similar issues with the new route so I can confirm this is the proper fix

I gave it a read and it improves on the current description.
Made a small edit removing the version.

Next step would be to convert this into the readme.md for the module
I would keep the: "Recommended modules" and "other modules" section.

Thanks for the effort

Perfect. Thanks for the improvement!

Makes sense. Thanks for the contribution

I merged https://git.drupalcode.org/project/simple_entity_merge/-/merge_requests/10 first so now this one has a conflict

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

Great cleanup :)

Thanks for the contribution.
I will roll a release to avoid the warning.

Indeed it makes sense to remove it. I think it comes from a time were the src folder of themes was not autoloaded.

Do you see why the pipeline is failing?

Sure

Oops, thanks for the reminder.

All layouts have a schema now + some schema fixes revealed by the functional test.
For the test to pass the Drupal 11 compatibility needs to be merged first
https://www.drupal.org/project/rocketship_core/issues/3536132 🐛 Compatibility with Drupal 11.2.2 Active

Locally the tests pass with dozens of deprecation warnings but we can work on those later and having them listed will ease the work.

Please review

rodrigoaguilera → created an issue.

I committed the suggested change to the 1.x branch from my phone.
Can you confirm it works?

Thanks for the detailed bug report and solution.

Sure. 1.0.1 released

Merged, thanks!

Hi
The code looks good except for the mix usage of [] and array. Can you use the short version in the new code? I wouldn't mind if you also convert some arrays around the code changed like $defaults.
Thanks for the contribution!

rodrigoaguilera → created an issue.

So when you uninstall this module your are able to see the unpublished content again?

I updated the patch for 6.0.x

As said before, there must be a better way to store the nonce

Added just the schema for rs_one_col in the MR.

rodrigoaguilera → created an issue.

I think the schema is correct now

I attended

Yes, totally a duplicate. Adding it as related issue.

I think RTBC might get more attention from a core maintainer to assign credit

rodrigoaguilera → created an issue.

#87 is an incomplete patch, here is the right version

Reroll attached of #85 applied agains Drupal 11.2.2 (with #356 from here 📌 Allow synced Layout override Translations: translating labels and inline blocks Needs work also applied)

Rebased the branch and moved the tests. Not sure why tests are failing.

Attached also you can fin the patch for Drupal 11.2.2 for composer

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

Just rebased the #22 patch and created an MR so it applies to Drupal 11.2.2
IS still needs updating.

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

In the context of OpenID Connect when using RS256 a "kid" is required on the JWKS endpoint and the header of the token.

If you try to pass the playground test with simple_oauth and this patch https://git.drupalcode.org/project/simple_oauth/-/merge_requests/171
It will complain about the "kid" being missing
https://openidconnect.net/

So the same kid needs to be in the JWKS endpoint and in the OpenIdConnectIdTokenResponse. I added one line there

    // Add required id_token claims.
     $builder = $this->getBuilder($accessToken, $userEntity);

     $builder = $builder->withHeader('kid', 'singlekey');
  

And I was able to go through all the steps in the playground

I have no issue using proper relative paths that start with ".."
Only absolute paths should start with /

I only tried on 6.0.0 not 5.2.x but seems that part hasn't changed much.
Closing because in case is not posible on the 5.2.x branch I doubt it will be added as a feature when is already present in 6.0.x.

Please reopen if you think the help text needs better guidance

Tested manually and it works great. Fixed some more phpcs issues

Thanks for the contribution!

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

I tested the latest MR and one thing I found weird is that the in the header array the indices for the rows can't be numeric.
Also when using multilevel the issues of having attributes in the headers cells is also solved, hence adding it as a related issue

rodrigoaguilera → created an issue.

+1

Ok, then I guess that config is needed if it is in the template and I imagine that since JS sniffing is deprecated in phpcs it won't be needed to exclude JS explictly as those files will be ignored by default by phpcs.

Thanks!

An example phpcs run showing the errors
https://git.drupalcode.org/project/font_awesome/-/jobs/5453102

lostcarpark → credited rodrigoaguilera → .

7.x is not supported anymore

I deleted the gitlab CI file as it was not correct and introduced in another issue.
There is a missing schema for the new settings as is indicated by the failing test

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

Thanks. Bear in mind that you should let others mark the issue as reviewed if you create the code

I don't know why I wasn't receiving notifications about this issue. I opted for keeping compatibility with 3.x of field_group
Thanks for the contribution

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

rodrigoaguilera → changed the visibility of the branch symfony_mailer-3388651-3388651-allow-customize-sender-2.x to hidden.

rodrigoaguilera → changed the visibility of the branch 3388651-allow-customize-sender-2.x to active.

rodrigoaguilera → changed the visibility of the branch 3388651-allow-customize-sender-2.x to hidden.

rodrigoaguilera → changed the visibility of the branch 3388651-allow-customize-sender to hidden.

Much clear now

I rebased the branch and committed the refactoring following your guidance. Thanks!

I checked out fa24d4c6407eb0bacaf08099be868b938717e6e6 and succesfully created a transport with the interface.
The code look good. I don't really use the architecture with the tagged services but I guess is important for flexibility opening the door for things like decorating.

Makes sense

Thanks for the contribution

I think your solution is better keeping all transports as tagged services. It is always great to have maintainer around. Thanks for you work on the module

Thanks for the contribution

rodrigoaguilera → created an issue.

Fixed all phpcs, phpstan and cspell issues.

Thanks!

Totally agree. I think this code was a copy of what devel was doing back then for inspecting entities

Don't know why the Merge button in gitlab is not working :(

Thanks for the contribution :)

Not sure 100% of use cases would want the date unchanged but I do believe is the correct default behaviour. I'm open to changes that would make this configurable.

Thanks for the contribution!

Indeed, I agree the tab title is too long.

Thanks for the contribution

rachel_norfolk → credited rodrigoaguilera → .

rodrigoaguilera → created an issue.

Applied the patch and the import worked. I believe that patch is the same as the MR. Setting it to RTBC.

Being one additional argument I guess it keeps compatibility with Drupal 10

rodrigoaguilera → created an issue.

I also fixed a bug with the emails to admins not being sent due to this line

    if($this->apply_for_role_config->get('send_user_approval_email')){

The idea would be to only use tokens in the future instead of the custom replacements logic but I think this is a small step in the right direction.

Please review

rodrigoaguilera → created an issue.

I ran the upgrade status scan on D10 and it reported the issues that I include in the MR.
I will perform more manual testing now with the module installed on Drupal 11.1.6 but usually upgrade_status catches all the issues so setting it to NR

rodrigoaguilera → created an issue.

Tests are failing because of the new default of FALSE for hide_untranslated_menu_links in the menu block.

I think the upgrade path is now ready so removing that tag

I won't be working on this in the near future

Merged 11.x into the branch.

I agree with @catch so I applied the idea of the

OnlyShowTranslatedMenuLinksInterface

instead of checking for a specific class.

If the pipelines return green then I will make $context optional and default to null when not passed. I see no reason for making it mandatory

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

Since the underlying data is an image field I think what is happening here is that the "Remove" is not triggered when clearing the canvas.

Not sure if that would be a viable solution or the button for removing the image should be exposed as some way of starting a fresh canvas deleting the previous image. Saving at this "clean" state will delete the image.

Let's discuss what should be the correct behavior

Sure