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.

Sign up for GitHub

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

Warning - headers getting sent twice #162 #163

Merged
merged 13 commits into from
Oct 4, 2021
Merged

Warning - headers getting sent twice #162 #163

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
d207f61
Update index.php
komen205 Oct 3, 2021
ab26aab
Update index.php
komen205 Oct 3, 2021
48e84e0
Update index.php
komen205 Oct 3, 2021
1bf5748
Update index.php
komen205 Oct 3, 2021
552df43
Render output refactor
komen205 Oct 3, 2021
24e24d7
Reducing code and refactoring renderOutput
komen205 Oct 4, 2021
8af7b7b
Update card.php
komen205 Oct 4, 2021
aeeefcd
Removing parameter from renderOutput
komen205 Oct 4, 2021
103da6b
Update index.php
komen205 Oct 4, 2021
1948749
Update card.php
komen205 Oct 4, 2021
39df778
Update src/card.php
komen205 Oct 4, 2021
4086de5
Update src/card.php
komen205 Oct 4, 2021
bfdd582
Merge branch 'main' into headers-getting-sent-twice
DenverCoder1 Oct 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions src/card.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,43 @@

declare(strict_types=1);

/**
* Set headers and echo response based on type
*
* @param string $output
* @param string If a message is a error
* @return void
*/
function renderOutput(string $output, bool $error = false): void
Copy link
Owner

Choose a reason for hiding this comment

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

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;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

{
$requestedType = $_REQUEST['type'] ?? 'svg';

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;
}
Copy link
Owner

Choose a reason for hiding this comment

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

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;
}


if ($requestedType === "png") {
echoAsPng($output);
exit;
}

echoAsSvg($output);
}


/**
* Convert date from Y-M-D to more human-readable format
*
Expand Down
32 changes: 3 additions & 29 deletions src/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,21 +13,13 @@
$dotenv = \Dotenv\Dotenv::createImmutable(dirname(__DIR__, 1));
$dotenv->safeLoad();

$requestedType = $_REQUEST['type'] ?? 'svg';

// if environment variables are not loaded, display error
if (!$_SERVER["TOKEN"] || !$_SERVER["USERNAME"]) {
$message = file_exists(dirname(__DIR__ . '.env', 1))
? "Missing token or username in config. Check Contributing.md for details."
: ".env was not found. Check Contributing.md for details.";

$card = generateErrorCard($message);
if ($requestedType === "png") {
echoAsPng($card);
}
echoAsSvg($card);

exit;
renderOutput(generateErrorCard($message));
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DenverCoder1 Strange... Why is this?

Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
renderOutput(generateErrorCard($message));
renderOutput($message);

}


Expand All @@ -50,26 +42,8 @@
$contributions = getContributionDates($contributionGraphs);
$stats = getContributionStats($contributions);
} catch (InvalidArgumentException $error) {
$card = generateErrorCard($error->getMessage());
if ($requestedType === "png") {
echoAsPng($card);
}
echoAsSvg($card);

exit;
renderOutput(generateErrorCard($error->getMessage()),true);
}
renderOutput(generateCard($stats));
Copy link
Owner

Choose a reason for hiding this comment

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

renderOutput(generateCard($stats));

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

Copy link
Owner

Choose a reason for hiding this comment

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

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.


if ($requestedType === "json") {
// set content type to JSON
header('Content-Type: application/json');
// echo JSON data for streak stats
echo json_encode($stats);
// exit
exit;
}

$card = generateCard($stats);
if ($requestedType === "png") {
echoAsPng($card);
}
echoAsSvg($card);