Problem/Motivation

It seems I was a bit careless when testing #3534874: Replace jQuery by vanilla JS (I probably forgot to flush the JS cache), and did not realize the obvious problem that the browser will block the GET request from going out since HTTP strictly forbids a GET request to have a body. Now NO ad will render AT ALL... Instead, we need to use query parameters.

Steps to reproduce

Proposed resolution

I will quickly implement the solution to the problem here (that will not require many changes).

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork ad-3544592

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

lrwebks created an issue. See original summary.

lrwebks opened merge request !36

lrwebks’s picture

lrwebks commented
Issue summary: View changes
lrwebks’s picture

lrwebks commented
Status: Active » Needs review

Works again, at least for me!

lrwebks’s picture

lrwebks commented
Status: Needs review » Needs work

@anybody and I are currently working on a fix that does not change the interface of the GET request too much, if at all.

lrwebks’s picture

lrwebks commented

@anybody: Do you remember what we wanted to change here? I understand that we do not want to change the GET request with query parameters, but I currently don't see any other way to provide all those specifications for what we want to the controller.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

@lrwebks as of https://stackoverflow.com/questions/978061/http-get-with-request-body I think the implementation might be correct. Does it work and do we have a tests now to check that it works? (We didn't see it broke before, right?)

lrwebks’s picture

lrwebks commented

Currently, 11.x-dev is broken because of this problem, but the current solution in here is working correctly. We currently don't have any tests in place *at all* for this module. Should we implement one for this here?

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Assigned: lrwebks » grevil
Status: Needs work » Needs review

Would be a good thing maybe for AI? What do you think @grevil? (Please review and then let's decide to merge this and create a separate issue for (ai generated) tests.

grevil’s picture

grevil commented
Status: Needs review » Needs work

Looks good! I'll create the tests here.

grevil’s picture

grevil commented
Title: Error fetching ad content: TypeError: Window.fetch: HEAD or GET Request cannot have a body » Error fetching ad content: TypeError: Window.fetch: HEAD or GET Request cannot have a body and add tests
Assigned: grevil » Unassigned
Status: Needs work » Needs review

Added tests. Although, one will fail, because of #3534872: JS advertisement type does not yet actually run the JS.

Not really worth it using AI to generate the tests. Took too many manual adjustments and therefore too much time...

Might need a better prompt.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Needs review » Needs work

@grevil can you put that test into #3534872: JS advertisement type does not yet actually run the JS then please?

grevil’s picture

grevil commented
Status: Needs work » Needs review

Also, there were no specific tests for this issue added... but I really don't wanna waste more time here. Please review.

grevil’s picture

grevil commented

Ok, done.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Needs review » Reviewed & tested by the community

Thanks all! Merged!

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Reviewed & tested by the community » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

grevil’s picture

grevil commented

Created a new release: https://www.drupal.org/project/ad/releases/11.0.0-alpha6.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.