Warning - headers getting sent twice #162 #163

komen205 · 2021-10-03T17:59:42Z

Description

Fixes #162

Type of change

Bug fix (added a non-breaking change which fixes an issue)

How Has This Been Tested?

Tested locally with a valid username
Tested locally with an invalid username
Ran tests with composer test
Added or updated test cases to test new features

Checklist:

I have checked to make sure no other pull requests are open for this issue
The code is properly formatted and is consistent with the existing code style
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings

Screenshots

DenverCoder1 · 2021-10-03T18:12:58Z

Doesn't work for error cards, generateErrorCard needs to be called instead of GenerateCard for some and it takes a message (string) instead of array (stats)

komen205 · 2021-10-03T18:29:52Z

What do you think about this solution?

DenverCoder1 · 2021-10-03T18:42:44Z

Both generateCard and generateErrorCard return a string might as well pass that in as an argument.

I'm thinking something like:

function renderOutput(string $output)
{
    $requestedType = $_REQUEST['type'] ?? 'svg';
    // set headers and echo response based on type
	...
}

And call it like

renderOutput(generateErrorCard($error->message));
or
renderOutput(generateCard($stats));

DenverCoder1 · 2021-10-03T20:12:09Z

JSON is another type to incorporate. All JSON outputs should use this function as well.

This may need to be redesigned to be written efficiently.

function renderOutput(string $output, bool $error)
{
    $type = $_REQUEST['type'] ?? 'svg';
    // set headers and echo response based on type
}

And call it like

renderOutput($error->message, true);
or
renderOutput(generateCard($stats), false);
or
renderOutput(json_encode($stats), false);

Send generateErrorCard($output) as SVG if error, just $output if no error.

Send json_encode(array("error" => $output)) as JSON if error, just $output.

Maybe I'll think about it a bit more later, feel free to share your thoughts on it.

DenverCoder1 · 2021-10-03T20:13:27Z

It may be worth considering a separate function for echoing stats than for echoing an error message since they are different variable types.

komen205 · 2021-10-03T22:04:53Z

What do you think about this solution? @DenverCoder1 Looks really neat to me!

DenverCoder1

$requestedType = $_REQUEST['type'] ?? 'svg'; is unnecessary on line 16 since it is now inside the function.
JSON output does not work. It should display the json encoding of $stats if successful, otherwise { "error": message }.

DenverCoder1 · 2021-10-03T22:22:30Z

src/index.php

+function renderOutput(string $output): void
+{


Can this function be moved to the card.php file?

src/index.php

komen205 · 2021-10-03T22:42:24Z

Does this work for you? We can move json to another function if needed, but it's pretty clear rn. @DenverCoder1

DenverCoder1 · 2021-10-03T22:53:18Z

src/index.php

 }
+renderOutput(generateCard($stats));


renderOutput(generateCard($stats));

This should probably go after the last line of the try-block (right after $stats is assigned)

As per the third comment (below), the generateCard/generateErrorCard can happen withing renderOutput, so this would end up being just

renderOutput($stats);

And for error outputs, you just pass the message.

DenverCoder1 · 2021-10-03T22:59:55Z

src/card.php

+    if ($requestedType === "json" && $error === false) {
+        // set content type to JSON
+        header('Content-Type: application/json');
+        // echo JSON data for streak stats
+        echo json_encode($output);
+        // exit
+        exit;
+    }
+
+    if ($requestedType === "json" && $error === true) {
+        // set content type to JSON
+        header('Content-Type: application/json');
+        // echo JSON error message
+        echo json_encode(array("error" => $output));
+        exit;
+    }


No need to repeat these lines, we can do it with nesting.

This could also be extracted to an echoAsJson method for consistency.

Suggested change

if ($requestedType === "json" && $error === false) {

// set content type to JSON

header('Content-Type: application/json');

// echo JSON data for streak stats

echo json_encode($output);

// exit

exit;

}

if ($requestedType === "json" && $error === true) {

// set content type to JSON

header('Content-Type: application/json');

// echo JSON error message

echo json_encode(array("error" => $output));

exit;

}

if ($requestedType === "json") {

$data = $error ? array("error" => $output) : $output;

// set content type to JSON

header('Content-Type: application/json');

// echo JSON data for streak stats

echo json_encode($data);

// exit

exit;

}

DenverCoder1 · 2021-10-03T23:05:58Z

src/card.php

+ * @param string If a message is a error
+ * @return void 
+ */
+function renderOutput(string $output, bool $error = false): void


Instead of passing $error, we could do (similar to your first idea), pass an array or string as the first parameter.

Union types are supported in PHP 8, so we can specify the parameter as array|string $output and then to check if it is an error or stats, we can use gettype, for example, gettype($output) === "string"

The card can then be generated like:

$card = gettype($output) === "string" ? generateErrorCard($output) : generateCard($output);

And the JSON data like:

$array = gettype($output) === "string" ? array("error" => $output) : $output;

That's a good idea, I'm not sure if that will improve readability specially for new comers. But we can go forward with that idea. @DenverCoder1

DenverCoder1 · 2021-10-04T08:47:55Z

src/index.php

-    echoAsSvg($card);
-
-    exit;
+    renderOutput(generateErrorCard($message));


The JSON won't work unless generateErrorCard/generateCard happens within the render function

@DenverCoder1 Strange... Why is this?

The JSON does not output use the SVG card to output. It just displays the plain message/stats.

DenverCoder1 · 2021-10-04T09:36:10Z

src/index.php

-    echoAsSvg($card);
-
-    exit;
+    renderOutput(generateErrorCard($message));


Suggested change

renderOutput(generateErrorCard($message));

renderOutput($message);

DenverCoder1 · 2021-10-04T09:36:23Z

src/index.php

@@ -49,27 +41,9 @@
    $contributionGraphs = getContributionGraphs($_REQUEST["user"]);
    $contributions = getContributionDates($contributionGraphs);
    $stats = getContributionStats($contributions);
+    renderOutput(generateCard($stats));


Suggested change

renderOutput(generateCard($stats));

renderOutput($stats);

DenverCoder1 · 2021-10-04T09:36:34Z

src/index.php

-    echoAsSvg($card);
-
-    exit;
+    renderOutput(generateErrorCard($error->getMessage()));


Suggested change

renderOutput(generateErrorCard($error->getMessage()));

renderOutput($error->getMessage());

DenverCoder1

Looks good! It's working now. Just a couple minor change suggestions and it will be ready to merge.

src/card.php

DenverCoder1 · 2021-10-04T09:46:12Z

src/card.php

+        // set content type to JSON
+        header('Content-Type: application/json');
+        // echo JSON error message
+        echo json_encode($data);


This can be extracted to a function for consistency with the other echo types.

/** * Displays an array as JSON * * @param array $array The array to output */ function echoAsJson(array $data): void { // set content type to JSON header("Content-Type: application/json"); // echo JSON data for streak stats echo json_encode($data); }

komen205 · 2021-10-04T09:54:22Z

Looks good now. I learned some things from this PR 👍

DenverCoder1

Just a couple type/comment changes. (You can commit suggestions through the GitHub website btw)

src/card.php

Co-authored-by: Jonah Lawrence <[email protected]>

DenverCoder1

Looks good now. I learned some things from this PR +1

Thanks for contributing! 🎉

Code review is a great way to learn, so I'm glad it helped.

Update index.php

d207f61

Update index.php

ab26aab

DenverCoder1 mentioned this pull request Oct 3, 2021

Change output errors as JSON when type set to json #165

Merged

13 tasks

komen205 added 2 commits October 3, 2021 22:59

Update index.php

48e84e0

Update index.php

1bf5748

DenverCoder1 reviewed Oct 3, 2021

View reviewed changes

Render output refactor

552df43

komen205 requested a review from DenverCoder1 October 3, 2021 22:53

DenverCoder1 reviewed Oct 3, 2021

View reviewed changes

Reducing code and refactoring renderOutput

24e24d7

komen205 requested a review from DenverCoder1 October 4, 2021 08:41

DenverCoder1 reviewed Oct 4, 2021

View reviewed changes

komen205 added 2 commits October 4, 2021 10:26

Update card.php

8af7b7b

Removing parameter from renderOutput

aeeefcd

komen205 requested a review from DenverCoder1 October 4, 2021 09:28

DenverCoder1 reviewed Oct 4, 2021

View reviewed changes

Update index.php

103da6b

komen205 requested a review from DenverCoder1 October 4, 2021 09:38

DenverCoder1 approved these changes Oct 4, 2021

View reviewed changes

Update card.php

1948749

komen205 requested a review from DenverCoder1 October 4, 2021 09:54

DenverCoder1 approved these changes Oct 4, 2021

View reviewed changes

src/card.php Outdated Show resolved Hide resolved

src/card.php Outdated Show resolved Hide resolved

komen205 and others added 2 commits October 4, 2021 11:15

Update src/card.php

39df778

Co-authored-by: Jonah Lawrence <[email protected]>

Update src/card.php

4086de5

Co-authored-by: Jonah Lawrence <[email protected]>

komen205 requested a review from DenverCoder1 October 4, 2021 10:15

DenverCoder1 approved these changes Oct 4, 2021

View reviewed changes

Merge branch 'main' into headers-getting-sent-twice

bfdd582

DenverCoder1 merged commit a74e2d8 into DenverCoder1:main Oct 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warning - headers getting sent twice #162 #163

Warning - headers getting sent twice #162 #163

komen205 commented Oct 3, 2021

DenverCoder1 commented Oct 3, 2021

komen205 commented Oct 3, 2021

DenverCoder1 commented Oct 3, 2021 •

edited

Loading

DenverCoder1 commented Oct 3, 2021

DenverCoder1 commented Oct 3, 2021

komen205 commented Oct 3, 2021

DenverCoder1 left a comment •

edited

Loading

DenverCoder1 Oct 3, 2021

komen205 commented Oct 3, 2021

DenverCoder1 Oct 3, 2021

DenverCoder1 Oct 3, 2021

DenverCoder1 Oct 3, 2021

DenverCoder1 Oct 3, 2021

komen205 Oct 4, 2021

DenverCoder1 Oct 4, 2021

komen205 Oct 4, 2021

DenverCoder1 Oct 4, 2021

DenverCoder1 Oct 4, 2021

DenverCoder1 Oct 4, 2021

DenverCoder1 Oct 4, 2021

DenverCoder1 left a comment

DenverCoder1 Oct 4, 2021

komen205 commented Oct 4, 2021

DenverCoder1 left a comment

DenverCoder1 left a comment

	renderOutput(generateErrorCard($message));
	renderOutput($message);

	renderOutput(generateErrorCard($error->getMessage()));
	renderOutput($error->getMessage());

Warning - headers getting sent twice #162 #163

Warning - headers getting sent twice #162 #163

Conversation

komen205 commented Oct 3, 2021

Description

Type of change

How Has This Been Tested?

Checklist:

Screenshots

DenverCoder1 commented Oct 3, 2021

komen205 commented Oct 3, 2021

DenverCoder1 commented Oct 3, 2021 • edited Loading

DenverCoder1 commented Oct 3, 2021

DenverCoder1 commented Oct 3, 2021

komen205 commented Oct 3, 2021

DenverCoder1 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

komen205 commented Oct 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DenverCoder1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

komen205 commented Oct 4, 2021

DenverCoder1 left a comment

Choose a reason for hiding this comment

DenverCoder1 left a comment

Choose a reason for hiding this comment

DenverCoder1 commented Oct 3, 2021 •

edited

Loading

DenverCoder1 left a comment •

edited

Loading