🇺🇦Ukraine @bohart

bohart → created an issue.

dinazaur → credited bohart → .

dinazaur → credited bohart → .

dinazaur → credited bohart → .

dinazaur → credited bohart → .

dinazaur → credited bohart → .

Hi @bramdriesen,
Here we’re: https://www.drupal.org/project/views_block_area/releases/2.0.0-rc1 →
‍‍‍‍‍‍ ‍‍
I’m not testing it much today. If/once any issue is raised, feel free to reach out to the issue queue :)
Our team plans to host the annual Drupal Contribution Weekend later this month.
And there is a chance we will look at and pick up a few more issues for this module.
‍‍‍‍‍‍ ‍‍
Thanks!

Christian, it normalizeQueryString for $_SERVER and $_REQUEST superglobals variables. We can just debug before and after overrideGlobals() to validate this (a quote from the previous comment):

overrideGlobals() does NOT change either $_GET["inline"] or $_REQUEST["inline"].

I have additionally run this code before and after overrideGlobals() executions:

        $values = [
          $event->getRequest()->query->all(),
          $event->getRequest()->query->get("inline"),
          $_GET["inline"],
          $_REQUEST["inline"],
          $_SERVER["REQUEST_URI"],
        ];

All variables are intact and unchanged. The code was run at clear Drupal11+reverse_proxy_header+media_entity_download (without any other modules).

At this point, there is no understanding of what exactly is changing by overrideGlobals().
Could you please provide:
- the exact variable / superglobal path / request object property which is changing?
- steps to reproduce the mentioned unnecessary redirect?

Thanks.

All unused statements, variables, and everything related to unused GuzzleHttp Client have been removed.
Committed to 1.1.x-dev branch. Those changes will be a part of the next module release (1.1.2).

Thanks for raising the issue!

Hi @penyaskito,

It sounds a bit strange to me, as overrideGlobals() is called during form builds and other modules routines, etc.
So (maybe) it is more of an issue with the media_entity_download module itself.

Still, I have installed the media_entity_download module and cannot reproduce the issue.
Either for inline or attachment GET parameters for media download links, no additional redirects occur.

overrideGlobals() does not change either $_GET["inline"] or $_REQUEST["inline"].
As well, as $event->getRequest()->request->get("inline"); returns null before and after overrideGlobals().

Could you please provide a few more steps to reproduce?
Or, maybe the issue is gone on the latest Drupal 11 + reverse_proxy_header / media_entity_download versions?

Looking forward,

All the needed changes were applied at #3425451 and #3434248.
A new Drupal 11 compatible release 1.1.0 was created.

Hi @harivenuv,
Thanks for your contribution and the detailed description of the merge request!

Please be aware that you changed the 1.0.x branch (instead of the 3425451-multiple-ips-returning branch).
1.0.x, 1.1.x, etc. (as the development branches) should remain intact in the forks.
You also have committed some unrelated changes (a line break in the comment).

In addition, there was no test coverage. I have added those (with some code refactoring).
Committed to 1.1.x-dev branch. Those will be a part of the next module release.

Thanks!

bohart → changed the visibility of the branch 3425451-multiple-ips-returning to hidden.

bohart → changed the visibility of the branch 1.0.x to hidden.

Committed to 1.1.x-dev branch.
Those will be a part of the next module release.

bohart → created an issue.

The provided MR fixes the issue.

It skips validations for the fields which are not presented in the form.
For example, the fields that are hidden in the "Manage form display".

bohart → created an issue.

abyss → credited bohart → .

abyss → credited bohart → .

greggles → credited bohart → .

Thanks for your contribution.

1) It looks like we should check for the first valid IP address in some foreach (instead of picking up the first address in the list and checking its validity), isn't it?

2) The maintainer of this module works only with Merge requests (instead of patch files).

Looking forward,

Thanks, fixed!

Committed to 2.3.x branch, the issue does not exist at 2.2.x branch.

Nice catch. Committed. Thanks!

bohart → changed the visibility of the branch 3427635-readme.md-code-snippet-helptext to active.

bohart → changed the visibility of the branch 3427635-readme.md-code-snippet-helptext to hidden.

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

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

2.3.x branch is green now. Thanks.

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

This one is tricky :)
The changes have been committed to both 2.2.x and 2.3.x branches.
Thanks!

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

This one is covered at 📌 Automated Drupal 11 compatibility fixes for reroute_email RTBC . Thanks.

The logo has been committed to 2.3.x branch.
It will be a part of 2.3.0-rc2 release (to be released soon).
Thanks!

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

All the changes needed for D11 have been committed at #3455294-21: Automated Drupal 11 compatibility fixes for reroute_email → .
Please see all the details on that issue.

Thanks.

Drupal 11 compatibility fixes have been committed to 2.3.x branch.
The minimum required Drupal version for 2.3.x branch is set to D10.2 (due to changes from 📌 [PHPUnit 10] Provide a static alternative to randomMachineName() and implement in data providers Fixed ).
The previous Drupal version (>9.4 and <10.2) is still supported within 2.2.x branch.

If no objection comes, all of those changes will be a part of 2.3.0-rc2 release (to be released soon).
Thanks!

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

abyss → credited bohart → .

Legacy comment: in July we provided the archive with high-quality photos from Drupal events in Lutsk (Ukraine).

abyss → credited bohart → .

bohart → created an issue. See original summary → .

Merged. The changes will be a part of the next module release. Thanks!

Merged. The changes will be a part of the next module release. Thanks!

After the latest changes at 8.x-1.x-dev branch, it looks like all changes at DuplicateRoleForm.php and .gitlab-ci.yml files must be removed from MR !7.

Marking this one as Needs work.

phpstan job will be fixed at 📌 Drupal 11 compatibility Active and PHPUnit in a separate issue.

The code has been committed and will be a part of the next module release.

@alt.dev, thanks!

1) Drupal 8 reached its EOL; so we can avoid its requirements.
A new arrow function syntax was introduced in PHP 7.4. So I bet it's okay to use the arrow syntax here.

2) I bet Role::loadMultiple should be replaced with entityTypeManager->getStorage('user_role')->loadMultiple();. Still, this is not a blocker here.

I'm going to merge those changes in the nearest weeks + create a new issue to have ci-cd for coding standards checks later.
Then, if no one object here, altogether will be released as a new 1.4 release of this module.

Thanks!

bohart → created an issue. See original summary → .

If a module does not have any dependencies, or the dependencies are solely other Drupal modules, then a composer.json is not required.
© drupal-org documentation

1) There is no need to commit composer.json to the module, as it will be generated by drupal-org packegist.
Are there any reasons for adding and maintaining the composer.json file?

2) The 2.2.x branch should not break the work with D9/D10, and its Drupal core version_requirement is not supposed to be lower than in 2.3.x (to follow semantic versioning).
All the changes must be committed either to 2.3.x only (if they break D9/D10) or to both 2.2.x/2.3.x (if they are valid for all D9/D10/D11).

@Rajeshreeputra, please do not create any new releases while we're proceeding with the topics above.

Null coalescing operator has been added in PHP 7, so we can safely use it on 2.3.x branch.

The change (an additional check) has been added to the 2.3.x development branch, and it will be a part of the next 2.3 release series.

If a module does not have any dependencies, or the dependencies are solely other Drupal modules, then a composer.json is not required.

© drupal-org documentation

Why do we need this change at all to this module?
The task description does not state the reasons for this change.
Could you please clarify? Thanks!

bohart → created an issue.

bohart → created an issue.