🇧🇪Belgium @tim-diels

Added first draft, test fails. So this needs work.
I think this is also a feature request? So changed the category.

This will need a Change Record also as I know already some modules that will have problems with this change?
I'll list the modules later when I did more tests.

I only added the code that was already provided. but fair enough I follow that I can not set own patches/MR as RTBC.

What do you need from us to continue in this issue? Some sort of CR so everybody knows what happend and that can be linked in this issue and in the release notes? Or were you thinking about something else?

tim-diels → changed the visibility of the branch 3400936-simplesitemap-version-compatibility to hidden.

I've used https://github.com/drupal/recommended-project/blob/10.2.x/composer.json as base to set up a site and this installed Drupal 10.2.6
Then I installed this module in 2 different ways and both without any problem.

1. Install domain with composer require 'drupal/domain:^2.0' and then this module with composer require 'drupal/domain_menu_access:^2.0' where dependencies are met => >works
2. Install this module with composer require 'drupal/domain_menu_access:^2.0' which also installs the dependencies (domain) => works

So please share the exact commands you use to install the module and also your composer.json so to see if there is a conflict.

To me it seems drupal/domain_menu_access dev-2.0.x requires drupal/domain_access * -> found drupal/domain_access[dev-1.x, dev-2.0.x, 1.0.0-alpha1, ..., 1.x-dev (alias of dev-1.x), 2.0.0-beta1, 2.0.x-dev (alias of dev-2.0.x)] but it does not match your minimum-stability. is more a setting you did not change in your composer.json to allow to install dev versions. It is however not encouraged to use dev versions, this is mostly at own risk. But if you do, you need to set your minimum-stability to match that. For example "minimum-stability": "dev"

@fathershawn Impressive work!

There are some PHPCS and PHPStan issues that needs resolving. Didn't had the time yet to fully test this.

Thanks @fathershawn

I've published the Change Record → .

@apaderno, thanks for the information. Looks like this MR is ready to go in. I updated the title to reflect more what this MR does.

@atul_ghate thanks for the verification.

Then we need to delete this from existing config with an update hook. This should be added to the MR.

Hey Pierre, much appreciated the time you put into this.
After our call, I went a different route that you suggested me.

For anyone else I've used a hook_preprocess_<PATTERN_ID> to alter the variant.
You can always ping me if you need more info.

Thank you for the work. Looks good.

I am not sure if the existing zone id from config is deleted automatically… is this something you tested? Otherwise we need to clean up existing config to delete the key and value. Could you please test and report back?

Great thanks 🙏

Looking forward for the work that you did. I already looked impressive what you showed me last time. We finished the dependent task so this can be continued.

This is not yet included in the default coding standards pipeline from Drupal. So please report what version you're using to get this report.

@shivamsen_12579 why are you taking an issue that is assigned to someone else? Please allow someone who is assigned to an issue to actually work on it.

Adding SKIP_ESLINT: '1' is not correct as we would actually want to run that.
Also this issue is outdated as this has been picked up within another issue and got merged.
Thanks for the work, sorry that I'm not including this from this issue.

@fathershawn

Looks like we've completed this. I'm going to merge it to dev.

We'll need to update the Change Record with the correct info. Is this something you would like to help with?

Closing this issue then as works as designed.

I think the code goes over the necessary records and delete them. You could do the same in an update hook?
Isn't the problem just that records with '[]' are faulty and should be deleted? Just wondering if I'm thinking correct.

Thanks for the reply, closing this issue.

Hi, thanks for the reply.

That's the general Cloudflare logo. I would rather have the more specific one for the Cloudflare Stream as you can see on https://developers.cloudflare.com/stream/
What do you think?

I added the svg.

Can we get this committed?

This is a very small fix, but still needed. +1 for RTBC

Is this still an issue?

Looks very good to me. RTBC.

@fathershawn

No problem at all.

1. Works as expected. Thanks.

2. For some reason this didn't work in the first place but does now with new tests. This should probably be a test in the future so we don't need to manually test this.

3. Looks much better and is descriptive enough to understand what is missing when trying to run the command.

Some things I changed or added:

• Adjusted some comments in the drush command so they were not defaults and use the ones if they existed.
• Changed some variable names as I think they fit better.
• Added config schema for cloudflare_stream_sync.settings

I think we're finished then.

Could you please test the changes I did? If all is fine, then we can commit this.

Sorry did something wrong. Removed a file accidently and changed status.

tim-diels → created an issue.

tim-diels → changed the visibility of the branch 3.0.x to hidden.

The 2.x MR 30 looks good. Tested this and works as expected with a nice message on the status page. Great work

Did you just edit the message your empty message to 'I am picking this issue and working on it.' and not post a new message? Please try to be more communicative in issue queues as it is otherwise very hard for others to follow?

Did you follow https://git.drupalcode.org/project/cloudflare_stream#usage for the setup?
I'll need more info to investigate what would be wrong. But i've tested the module last week and still worked for me. So could be you've missed some steps?

Did you configure the display settings of the media? There you have the settings for the CloudFlare Stream video player.

And please do not use critical as this didn't break your site.

This should actually be just an update hook to resave the items to the new structure instead of adding code that is not needed anymore afterwards?

Please show the settings from the view to allow people to help you. You probably have multiple weight fields and only have hidden one?

As the module moved to 2.x and the patches also, lets set that in the version of this issue report also.
FYI: I did not test the functionality so can't confirm this works and you should read previous comments.

You need to provide more info if anyone would like to help you. With the current information, it is not clear how you have set things up and where it goes wrong. And you should test if the same behaviour exists in the dev branch and report against the dev branch.
As you now report against an older version, did you try updating?

@Madhukar K Could you explain your empty comment? What are your plans on taking this up? If no reaction is given in a certain time, I'll unassign so someone else can pick up.

You need to update to the 3.x version. It is not reproducable anymore with the next versions:

We're on the next versions:

• domain_simple_sitemap 3.0.0-beta1
• simple_sitemap 4.1.9

And if you see there, your fix is also not correct.

Also the bug report is not clear. You talk about a faulty declaration but also provide a (partially) PHP CS report?

FILE: modules/contrib/domain_simple_sitemap/src/Controller/DomainSimpleSitemapController.php
-----------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-----------------------------------------------------------------------------------------------------------------
16 | WARNING | The class short comment should describe what the class does and not simply repeat the class name
-----------------------------------------------------------------------------------------------------------------

And if you want to solve the CS issue, please read carefully how to do that as your provided solution is not correct. The comment should be descriptive and not just being fixed with adding an 'of' between.

I would say we close this issue? At least it needs work.

Oh for the 3.0.x changes I tested the Drush command, and I got a straight error.

AssertionError: Cannot load the "media_type" entity with NULL ID. in /var/www/html/web/core/lib/Drupal/Core/Entity/EntityStorageBase.php on line 261 #0 /var/www/html/web/core/lib/Drupal/Core/Entity/EntityStorageBase.php(261): assert(false, 'Cannot load the...')

Maybe we should give a nicer error?

Thanks @FatherShawn for looking into this and pinging me on Slack and also thanks for the nice call we had a few days ago. I'm looking forward to get this landed.

As you mentioned, we should mark the cloudflare_stream_hosted_video as obsolete and provide guidance on how to upgrade to 3.0.x. We could add this as a Change Record to this module.

2.x changes:

When adding the changes from MR 30 for 2.x, the site throws an error:

Drupal\Core\Extension\InfoParserException: Extension Cloudflare Stream - Hosted video (modules/contrib/cloudflare_stream/modules/cloudflare_stream_hosted_video/cloudflare_stream_hosted_video.info.yml) has 'lifecycle: obsolete' but is missing a 'lifecycle_link' entry. in Drupal\Core\Extension\InfoParserDynamic->parse() (line 95 of core/lib/Drupal/Core/Extension/InfoParserDynamic.php).

For deprecated and obsolete lifecycle states, a lifecycle_link is required which specifies a URL to the documentation to display to users. This helps orient site builders as to what do. See https://www.drupal.org/node/3215042 →

We could use the Change Record for the lifecycle_link.

3.0.x changes:

When adding the changes from MR 31 for 3.0.x, I'm missing functionality:

• The template is not used anymore so the description is not shown.

For the rest I'm not having any functional problems. I'll look deeper into the code later.

tim-diels → created an issue.

tim-diels → created an issue.

I've tested the code and it works as expected, so for me it is RTBC. I created a new MR against 2.x with same code and tested it.

This should be targeted towards the 2.x branch.

Provided MR.

tim-diels → created an issue.

tim-diels → created an issue.

Made MR.

Added.

tim-diels → created an issue.

tim-diels → created an issue.

Thanks for the report and work done, fixed.

Seems logical to me to add this but could not reproduce it to be honest. I fixed where the $text variable was outside the function somehow and follow more the domain_access logic.

Add patch as MR, so hiding patch.

tim-diels → created an issue.

I've tested this myself and works as expected.

Adding this as a MR to allow to run the pipelines.

Hey Peter, looks good to me. But we need to have existing installs updated to have the new permissions. So we need an update hook for this.

Hmm can not reproduce this anymore. Could you please test with the latest version from this module but also from domain to see if you can still reproduce this? The code change does look ok to me.

tim-diels → created an issue.

Added menu_block as dev requirement and reduces errors to:

 ------ ------------------------------------------------------------------------------------------ 
  Line   modules/domain_menu_access_menu_block/src/Plugin/Block/DomainMenuAccessMenuMenuBlock.php  
 ------ ------------------------------------------------------------------------------------------ 
  180    \Drupal calls should be avoided in classes, use dependency injection                      
         instead                                                                                   
 ------ ------------------------------------------------------------------------------------------ 
 ------ ---------------------------------------------------------------------- 
  Line   src/Plugin/Block/DomainMenuAccessMenuBlock.php                        
 ------ ---------------------------------------------------------------------- 
  68     \Drupal calls should be avoided in classes, use dependency injection  
         instead                                                               
 ------ ---------------------------------------------------------------------- 
 [ERROR] Found 2 errors      

tim-diels → created an issue.

tim-diels → created an issue.

tim-diels → created an issue.

Added to 1.0.0 release

tim-diels → created an issue.

tim-diels → created an issue.

tim-diels → created an issue.

Closing as no response

Added

tim-diels → created an issue.

There were issues with forking and gitlab. So didn't work with a MR.

tim-diels → created an issue.

Added in 2.0.3 release.

As suggested by kristiaanvandeneynde → , it is better to remove the maintainer info from the README as there is more info in composer.json.

Sorry @Hritick, but hijacking the issue back to me as I want to finish the work here.

There we go, all green.

I understand the issue, just adding the info here for the record. So if others are having issues, they can see what happened.
Thanks for the info anyway.

With adding the composer.json in another issue, the original issue here is gone.
A new one has appeared.

tim-diels → created an issue.