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

Fixed an error in the routing chapter #6576

Closed

Conversation

vanbrabantf
Copy link

Q A
Doc fix? yes
New docs? no
Applies to all
Fixed tickets none

The $name variable isn't initiated.

The `$name` variable isn't initiated.
@javiereguiluz
Copy link
Member

I'm not sure if this change is really needed. The code that renders templates uses the extract() PHP function to make the variables available in the template, right?

ob_start();
extract($request->query->all(), EXTR_SKIP);  // <-- this is where vars are made available
include sprintf(__DIR__.'/../src/pages/%s.php', $map[$path]);
$response = new Response(ob_get_clean());

@vanbrabantf
Copy link
Author

I thought the same but when running the code (on windows 10 with PHP7.0.2, not sure if this matters), I do get the Notice: Undefined variable: name Notice.

Do you want me to share the code provided up to this point in the article (To make it easier to get an overview) ?

@xabbuh
Copy link
Member

xabbuh commented May 19, 2016

It looks like you did forget to include the name URL parameter when accessing the page in your browser. However, we may want to think about adding a check that displays a different sentence when no name was given. What do you think?

@vanbrabantf
Copy link
Author

In the rest of the article there is always a backup, (world is used as alternative). I guess it's a matter of how much you would like to cover in the article.

If you want I can open another pull request with a catch in it.

@wouterj
Copy link
Member

wouterj commented May 21, 2016

👍 for adding a default value.

@xabbuh
Copy link
Member

xabbuh commented May 21, 2016

I just noticed that we already have #6419 which does that. So I think we sadly have to close this as a duplicate and merge #6419 instead.

@vanbrabantf
Copy link
Author

Ok, great.
Thanks for the update!

@wouterj
Copy link
Member

wouterj commented May 21, 2016

Thanks for your responses @vanbrabantf. Unfortunately, I'm going to close this one in favor of #6419. I hope to see you back with a PR that we can merge in the future!

@wouterj wouterj closed this May 21, 2016
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.

4 participants