Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add patch from https://www.drupal.org/project/group/issues/2853001 to… #497

Merged
merged 3 commits into from
Nov 5, 2024

Conversation

finnlewis
Copy link
Member

This an issue, again!

Just like mentioned here: https://www.drupal.org/project/responsive_preview/issues/3302861

Which takes us back to https://www.drupal.org/project/group/issues/2853001

The latest patch on there seems to suppress the problem.

Steps to reporoduce:

install localgov_microsites 4.0.0 :)
create a microsite and assign an admin user to the microsite
log into microsite as admin user
create a newsroom
See

Error message

Warning: Undefined array key "preview" in Drupal\responsive_preview\ResponsivePreview->formAlter() (line 221 of modules/contrib/responsive_preview/src/ResponsivePreview.php).

Warning: Trying to access array offset on value of type null in Drupal\responsive_preview\ResponsivePreview->formAlter() (line 221 of modules/contrib/responsive_preview/src/ResponsivePreview.php).

Solution: apply this patch https://www.drupal.org/files/issues/2023-09-25/group2-2853001-26.patch

… supress warning when creating new content.
@finnlewis
Copy link
Member Author

finnlewis commented Sep 18, 2024

@ekes or @stephen-cox can you help me work out why this test is failing?

When I test it manually, it behaves as expected.

Logging into the control site forwards the user to /admin

Logging into a microsite forwards the user to /group/1

So why would the test be behaving differently?

https://github.com/localgovdrupal/localgov_microsites_group/actions/runs/10921854523/job/30315030208?pr=497#step:5:16

1) Drupal\Tests\localgov_microsites_group\Functional\LoginTest::testLoginRedirect
Behat\Mink\Exception\ExpectationException: Current page is "/user/2", but "/admin" expected.

/var/www/html/web/core/tests/Drupal/Tests/WebAssert.php:591
/var/www/html/web/core/tests/Drupal/Tests/WebAssert.php:779
/var/www/html/web/modules/contrib/localgov_microsites_group/tests/src/Functional/LoginTest.php:80
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:729

@ekes
Copy link
Member

ekes commented Sep 24, 2024

So why would the test be behaving differently?

If the same code passes locally, and fails here. I think something about the way the test is run meaning that the domain negotiator does not think there is an active domain:-

that's the reason it wouldn't redirect to either admin or the group page.

or it getting cached differently
https://git.drupalcode.org/project/domain/-/blob/2.0.x/domain/src/DomainNegotiator.php?ref_type=heads#L146
but the negotiation is
https://git.drupalcode.org/project/domain/-/blob/2.0.x/domain/src/DomainNegotiator.php?ref_type=heads#L164

I guess in group site tests we set the site, is it not defaulting to having the domain of the control site at other times? How is that set when running the test, environment variables?

@ekes
Copy link
Member

ekes commented Sep 24, 2024

@ekes
Copy link
Member

ekes commented Sep 24, 2024

So locally it installs with

uuid: 6ddda674-911f-4efb-ad0c-2da28bca56c2
langcode: en
status: true
dependencies: {  }
id: localgov_microsites_ddev_site
domain_id: 2467579
hostname: localgov-microsites.ddev.site
name: 'Drush Site-Install'
scheme: variable
weight: -1
is_default: true

And during a test the ID is: example_com but the hostname: is updated to web or whatever I'm asking for; and it's the default.

What is it in the github test, and why isn't it returned if it's the default anyway. What's creating and updating this.

@finnlewis
Copy link
Member Author

I think we should merge and release this, and follow up on the odd failing tests on #503

@finnlewis finnlewis self-assigned this Oct 29, 2024
Copy link
Member

@ekes ekes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just read the issue. As I understand it this patch fixes the issue for us for now, but what will finally get merged might be a different approach. But I don't think changing from one to the other is going to be an issue, and people on the issue seem to be happy enough with this approach now too.

@stephen-cox stephen-cox merged commit ea24200 into 4.x Nov 5, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants