Adding Chrome Web Store PHP Hello World

Adding Chrome Web Store PHP Hello World

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=56936

[ericbidelman, 2010-08-19 21:11:11]

Here is the PHP hello world

[kurrik.chromium, 2010-08-19 23:08:49]

Looks pretty good, mostly just nits about licensing + formatting.

On 2010/08/19 21:11:11, ericbidelman wrote:
> Here is the PHP hello world

[kurrik.chromium, 2010-08-19 23:09:01]

http://codereview.chromium.org/3122030/diff/1/5
File chrome/common/extensions/docs/examples/apps/hello-php/index.php (right):

http://codereview.chromium.org/3122030/diff/1/5#newcode27
chrome/common/extensions/docs/examples/apps/hello-php/index.php:27:
require_once('lib/oauth/OAuth.php');
I've been told in the past that require_once should not be called this way - you
just write

require_once 'lib/oauth/OAuth.php';

See
http://code.google.com/p/opensocial-php-client/source/browse/trunk/src/osapi/...
for an example.

http://codereview.chromium.org/3122030/diff/1/5#newcode31
chrome/common/extensions/docs/examples/apps/hello-php/index.php:31: $selfUrl =
"http://{$_SERVER['HTTP_HOST']}{$_SERVER['PHP_SELF']}";
Depending on how thorough you want to be, you may consider checking the scheme
as well.  Your OAuth library does this:

$scheme = (!isset($_SERVER['HTTPS']) || $_SERVER['HTTPS'] != "on") ? 'http' :
'https';

http://codereview.chromium.org/3122030/diff/1/5#newcode43
chrome/common/extensions/docs/examples/apps/hello-php/index.php:43: const TOKEN
= '1/mlzu7PdZlGXp3GCjuD1QaCEUuiFVJSXDHYu8yb86o1I';
Don't use your own token.

http://codereview.chromium.org/3122030/diff/1/5#newcode44
chrome/common/extensions/docs/examples/apps/hello-php/index.php:44: const
TOKEN_SECRET = 'NcBoCFErCMdeiUJJBqIhlaby';
Don't use your own token secret.

http://codereview.chromium.org/3122030/diff/1/5#newcode60
chrome/common/extensions/docs/examples/apps/hello-php/index.php:60: * @param
string $request OAuthRequest object containing the signed request to make.
This line wraps in the review - is it > 80 chars?

http://codereview.chromium.org/3122030/diff/1/5#newcode62
chrome/common/extensions/docs/examples/apps/hello-php/index.php:62: * @param
bool $returnResponseHeaders True if resp. headers should be returned.
This line wraps as well.

http://codereview.chromium.org/3122030/diff/1/5#newcode109
chrome/common/extensions/docs/examples/apps/hello-php/index.php:109:
$request->sign_request($this->signatureMethod, $this->consumer, $this->token);
This line wraps.

http://codereview.chromium.org/3122030/diff/1/5#newcode123
chrome/common/extensions/docs/examples/apps/hello-php/index.php:123:
$openid->required = array('namePerson/first', 'namePerson/last',
'contact/email');
This line wraps.

http://codereview.chromium.org/3122030/diff/1/5#newcode135
chrome/common/extensions/docs/examples/apps/hello-php/index.php:135: echo
$e->getMessage();
Should you exit here?

http://codereview.chromium.org/3122030/diff/1/5#newcode166
chrome/common/extensions/docs/examples/apps/hello-php/index.php:166: This user
has <span class="<%= accessLevel.toLowerCase() %>"><%= accessLevel %></span>
access to this application ( appId: <%= appId %> )
Ignoring the wraps here because it's pretty hard to make HTML conform to 80
character widths

http://codereview.chromium.org/3122030/diff/1/5#newcode192
chrome/common/extensions/docs/examples/apps/hello-php/index.php:192: // Simple
JavaScript Templating
Put a copy of this in the NOTICE file

http://codereview.chromium.org/3122030/diff/1/5#newcode249
chrome/common/extensions/docs/examples/apps/hello-php/index.php:249: var url =
[form.action, '&user_id=', encodeURIComponent(userId)].join('');
You may want to consider fixing this wrap though, and the next one.

http://codereview.chromium.org/3122030/diff/1/11
File chrome/common/extensions/docs/examples/apps/hello-php/popuplib.js (right):

http://codereview.chromium.org/3122030/diff/1/11#newcode3
chrome/common/extensions/docs/examples/apps/hello-php/popuplib.js:3: // 
Licensed under the Apache License, Version 2.0 (the "License");
Put a copy of this in the NOTICE file

[ericbidelman, 2010-08-20 00:08:04]

http://codereview.chromium.org/3122030/diff/1/5
File chrome/common/extensions/docs/examples/apps/hello-php/index.php (right):

http://codereview.chromium.org/3122030/diff/1/5#newcode27
chrome/common/extensions/docs/examples/apps/hello-php/index.php:27:
require_once('lib/oauth/OAuth.php');
On 2010/08/19 23:09:01, kurrik.chromium wrote:
> I've been told in the past that require_once should not be called this way -
you
> just write
> 
> require_once 'lib/oauth/OAuth.php';
> 
> See
>
http://code.google.com/p/opensocial-php-client/source/browse/trunk/src/osapi/...
> for an example.

Done.

http://codereview.chromium.org/3122030/diff/1/5#newcode31
chrome/common/extensions/docs/examples/apps/hello-php/index.php:31: $selfUrl =
"http://{$_SERVER['HTTP_HOST']}{$_SERVER['PHP_SELF']}";
On 2010/08/19 23:09:01, kurrik.chromium wrote:
> Depending on how thorough you want to be, you may consider checking the scheme
> as well.  Your OAuth library does this:
> 
> $scheme = (!isset($_SERVER['HTTPS']) || $_SERVER['HTTPS'] != "on") ? 'http' :
> 'https';

Done.

http://codereview.chromium.org/3122030/diff/1/5#newcode43
chrome/common/extensions/docs/examples/apps/hello-php/index.php:43: const TOKEN
= '1/mlzu7PdZlGXp3GCjuD1QaCEUuiFVJSXDHYu8yb86o1I';
On 2010/08/19 23:09:01, kurrik.chromium wrote:
> Don't use your own token.

Uploaded the wrong directory :(

http://codereview.chromium.org/3122030/diff/1/5#newcode44
chrome/common/extensions/docs/examples/apps/hello-php/index.php:44: const
TOKEN_SECRET = 'NcBoCFErCMdeiUJJBqIhlaby';
On 2010/08/19 23:09:01, kurrik.chromium wrote:
> Don't use your own token secret.

Uploaded the wrong directory :(

http://codereview.chromium.org/3122030/diff/1/5#newcode60
chrome/common/extensions/docs/examples/apps/hello-php/index.php:60: * @param
string $request OAuthRequest object containing the signed request to make.
On 2010/08/19 23:09:01, kurrik.chromium wrote:
> This line wraps in the review - is it > 80 chars?

Done.

http://codereview.chromium.org/3122030/diff/1/5#newcode62
chrome/common/extensions/docs/examples/apps/hello-php/index.php:62: * @param
bool $returnResponseHeaders True if resp. headers should be returned.
On 2010/08/19 23:09:01, kurrik.chromium wrote:
> This line wraps as well.

Done.

http://codereview.chromium.org/3122030/diff/1/5#newcode109
chrome/common/extensions/docs/examples/apps/hello-php/index.php:109:
$request->sign_request($this->signatureMethod, $this->consumer, $this->token);
On 2010/08/19 23:09:01, kurrik.chromium wrote:
> This line wraps.

Done.

http://codereview.chromium.org/3122030/diff/1/5#newcode123
chrome/common/extensions/docs/examples/apps/hello-php/index.php:123:
$openid->required = array('namePerson/first', 'namePerson/last',
'contact/email');
On 2010/08/19 23:09:01, kurrik.chromium wrote:
> This line wraps.

Done.

http://codereview.chromium.org/3122030/diff/1/5#newcode135
chrome/common/extensions/docs/examples/apps/hello-php/index.php:135: echo
$e->getMessage();
On 2010/08/19 23:09:01, kurrik.chromium wrote:
> Should you exit here?

Done.

http://codereview.chromium.org/3122030/diff/1/5#newcode166
chrome/common/extensions/docs/examples/apps/hello-php/index.php:166: This user
has <span class="<%= accessLevel.toLowerCase() %>"><%= accessLevel %></span>
access to this application ( appId: <%= appId %> )
On 2010/08/19 23:09:01, kurrik.chromium wrote:
> Ignoring the wraps here because it's pretty hard to make HTML conform to 80
> character widths

Done.

http://codereview.chromium.org/3122030/diff/1/5#newcode192
chrome/common/extensions/docs/examples/apps/hello-php/index.php:192: // Simple
JavaScript Templating
On 2010/08/19 23:09:01, kurrik.chromium wrote:
> Put a copy of this in the NOTICE file

Done.

http://codereview.chromium.org/3122030/diff/1/5#newcode249
chrome/common/extensions/docs/examples/apps/hello-php/index.php:249: var url =
[form.action, '&user_id=', encodeURIComponent(userId)].join('');
On 2010/08/19 23:09:01, kurrik.chromium wrote:
> You may want to consider fixing this wrap though, and the next one.

Done.

[kurrik.chromium, 2010-08-20 01:36:38]

LGTM.  Should I commit?


On 2010/08/20 00:08:04, ericbidelman wrote:
> http://codereview.chromium.org/3122030/diff/1/5
> File chrome/common/extensions/docs/examples/apps/hello-php/index.php (right):
> 
> http://codereview.chromium.org/3122030/diff/1/5#newcode27
> chrome/common/extensions/docs/examples/apps/hello-php/index.php:27:
> require_once('lib/oauth/OAuth.php');
> On 2010/08/19 23:09:01, kurrik.chromium wrote:
> > I've been told in the past that require_once should not be called this way -
> you
> > just write
> > 
> > require_once 'lib/oauth/OAuth.php';
> > 
> > See
> >
>
http://code.google.com/p/opensocial-php-client/source/browse/trunk/src/osapi/...
> > for an example.
> 
> Done.
> 
> http://codereview.chromium.org/3122030/diff/1/5#newcode31
> chrome/common/extensions/docs/examples/apps/hello-php/index.php:31: $selfUrl =
> "http://{$_SERVER['HTTP_HOST']}{$_SERVER['PHP_SELF']}";
> On 2010/08/19 23:09:01, kurrik.chromium wrote:
> > Depending on how thorough you want to be, you may consider checking the
scheme
> > as well.  Your OAuth library does this:
> > 
> > $scheme = (!isset($_SERVER['HTTPS']) || $_SERVER['HTTPS'] != "on") ? 'http'
:
> > 'https';
> 
> Done.
> 
> http://codereview.chromium.org/3122030/diff/1/5#newcode43
> chrome/common/extensions/docs/examples/apps/hello-php/index.php:43: const
TOKEN
> = '1/mlzu7PdZlGXp3GCjuD1QaCEUuiFVJSXDHYu8yb86o1I';
> On 2010/08/19 23:09:01, kurrik.chromium wrote:
> > Don't use your own token.
> 
> Uploaded the wrong directory :(
> 
> http://codereview.chromium.org/3122030/diff/1/5#newcode44
> chrome/common/extensions/docs/examples/apps/hello-php/index.php:44: const
> TOKEN_SECRET = 'NcBoCFErCMdeiUJJBqIhlaby';
> On 2010/08/19 23:09:01, kurrik.chromium wrote:
> > Don't use your own token secret.
> 
> Uploaded the wrong directory :(
> 
> http://codereview.chromium.org/3122030/diff/1/5#newcode60
> chrome/common/extensions/docs/examples/apps/hello-php/index.php:60: * @param
> string $request OAuthRequest object containing the signed request to make.
> On 2010/08/19 23:09:01, kurrik.chromium wrote:
> > This line wraps in the review - is it > 80 chars?
> 
> Done.
> 
> http://codereview.chromium.org/3122030/diff/1/5#newcode62
> chrome/common/extensions/docs/examples/apps/hello-php/index.php:62: * @param
> bool $returnResponseHeaders True if resp. headers should be returned.
> On 2010/08/19 23:09:01, kurrik.chromium wrote:
> > This line wraps as well.
> 
> Done.
> 
> http://codereview.chromium.org/3122030/diff/1/5#newcode109
> chrome/common/extensions/docs/examples/apps/hello-php/index.php:109:
> $request->sign_request($this->signatureMethod, $this->consumer, $this->token);
> On 2010/08/19 23:09:01, kurrik.chromium wrote:
> > This line wraps.
> 
> Done.
> 
> http://codereview.chromium.org/3122030/diff/1/5#newcode123
> chrome/common/extensions/docs/examples/apps/hello-php/index.php:123:
> $openid->required = array('namePerson/first', 'namePerson/last',
> 'contact/email');
> On 2010/08/19 23:09:01, kurrik.chromium wrote:
> > This line wraps.
> 
> Done.
> 
> http://codereview.chromium.org/3122030/diff/1/5#newcode135
> chrome/common/extensions/docs/examples/apps/hello-php/index.php:135: echo
> $e->getMessage();
> On 2010/08/19 23:09:01, kurrik.chromium wrote:
> > Should you exit here?
> 
> Done.
> 
> http://codereview.chromium.org/3122030/diff/1/5#newcode166
> chrome/common/extensions/docs/examples/apps/hello-php/index.php:166: This user
> has <span class="<%= accessLevel.toLowerCase() %>"><%= accessLevel %></span>
> access to this application ( appId: <%= appId %> )
> On 2010/08/19 23:09:01, kurrik.chromium wrote:
> > Ignoring the wraps here because it's pretty hard to make HTML conform to 80
> > character widths
> 
> Done.
> 
> http://codereview.chromium.org/3122030/diff/1/5#newcode192
> chrome/common/extensions/docs/examples/apps/hello-php/index.php:192: // Simple
> JavaScript Templating
> On 2010/08/19 23:09:01, kurrik.chromium wrote:
> > Put a copy of this in the NOTICE file
> 
> Done.
> 
> http://codereview.chromium.org/3122030/diff/1/5#newcode249
> chrome/common/extensions/docs/examples/apps/hello-php/index.php:249: var url =
> [form.action, '&user_id=', encodeURIComponent(userId)].join('');
> On 2010/08/19 23:09:01, kurrik.chromium wrote:
> > You may want to consider fixing this wrap though, and the next one.
> 
> Done.

[ericbidelman, 2010-08-20 01:38:30]

Yes, please.

On Thu, Aug 19, 2010 at 6:36 PM, <kurrik@chromium.org> wrote:

> LGTM.  Should I commit?
>
>
>
> On 2010/08/20 00:08:04, ericbidelman wrote:
>
>> http://codereview.chromium.org/3122030/diff/1/5
>> File chrome/common/extensions/docs/examples/apps/hello-php/index.php
>> (right):
>>
>
>  http://codereview.chromium.org/3122030/diff/1/5#newcode27
>> chrome/common/extensions/docs/examples/apps/hello-php/index.php:27:
>> require_once('lib/oauth/OAuth.php');
>> On 2010/08/19 23:09:01, kurrik.chromium wrote:
>> > I've been told in the past that require_once should not be called this
>> way -
>> you
>> > just write
>> >
>> > require_once 'lib/oauth/OAuth.php';
>> >
>> > See
>> >
>>
>
>
>
http://code.google.com/p/opensocial-php-client/source/browse/trunk/src/osapi/...
>
>> > for an example.
>>
>
>  Done.
>>
>
>  http://codereview.chromium.org/3122030/diff/1/5#newcode31
>> chrome/common/extensions/docs/examples/apps/hello-php/index.php:31:
>> $selfUrl =
>> "http://{$_SERVER['HTTP_HOST']}{$_SERVER['PHP_SELF']}";
>> On 2010/08/19 23:09:01, kurrik.chromium wrote:
>> > Depending on how thorough you want to be, you may consider checking the
>>
> scheme
>
>> > as well.  Your OAuth library does this:
>> >
>> > $scheme = (!isset($_SERVER['HTTPS']) || $_SERVER['HTTPS'] != "on") ?
>> 'http'
>>
> :
>
>> > 'https';
>>
>
>  Done.
>>
>
>  http://codereview.chromium.org/3122030/diff/1/5#newcode43
>> chrome/common/extensions/docs/examples/apps/hello-php/index.php:43: const
>>
> TOKEN
>
>> = '1/mlzu7PdZlGXp3GCjuD1QaCEUuiFVJSXDHYu8yb86o1I';
>> On 2010/08/19 23:09:01, kurrik.chromium wrote:
>> > Don't use your own token.
>>
>
>  Uploaded the wrong directory :(
>>
>
>  http://codereview.chromium.org/3122030/diff/1/5#newcode44
>> chrome/common/extensions/docs/examples/apps/hello-php/index.php:44: const
>> TOKEN_SECRET = 'NcBoCFErCMdeiUJJBqIhlaby';
>> On 2010/08/19 23:09:01, kurrik.chromium wrote:
>> > Don't use your own token secret.
>>
>
>  Uploaded the wrong directory :(
>>
>
>  http://codereview.chromium.org/3122030/diff/1/5#newcode60
>> chrome/common/extensions/docs/examples/apps/hello-php/index.php:60: *
>> @param
>> string $request OAuthRequest object containing the signed request to make.
>> On 2010/08/19 23:09:01, kurrik.chromium wrote:
>> > This line wraps in the review - is it > 80 chars?
>>
>
>  Done.
>>
>
>  http://codereview.chromium.org/3122030/diff/1/5#newcode62
>> chrome/common/extensions/docs/examples/apps/hello-php/index.php:62: *
>> @param
>> bool $returnResponseHeaders True if resp. headers should be returned.
>> On 2010/08/19 23:09:01, kurrik.chromium wrote:
>> > This line wraps as well.
>>
>
>  Done.
>>
>
>  http://codereview.chromium.org/3122030/diff/1/5#newcode109
>> chrome/common/extensions/docs/examples/apps/hello-php/index.php:109:
>> $request->sign_request($this->signatureMethod, $this->consumer,
>> $this->token);
>> On 2010/08/19 23:09:01, kurrik.chromium wrote:
>> > This line wraps.
>>
>
>  Done.
>>
>
>  http://codereview.chromium.org/3122030/diff/1/5#newcode123
>> chrome/common/extensions/docs/examples/apps/hello-php/index.php:123:
>> $openid->required = array('namePerson/first', 'namePerson/last',
>> 'contact/email');
>> On 2010/08/19 23:09:01, kurrik.chromium wrote:
>> > This line wraps.
>>
>
>  Done.
>>
>
>  http://codereview.chromium.org/3122030/diff/1/5#newcode135
>> chrome/common/extensions/docs/examples/apps/hello-php/index.php:135: echo
>> $e->getMessage();
>> On 2010/08/19 23:09:01, kurrik.chromium wrote:
>> > Should you exit here?
>>
>
>  Done.
>>
>
>  http://codereview.chromium.org/3122030/diff/1/5#newcode166
>> chrome/common/extensions/docs/examples/apps/hello-php/index.php:166: This
>> user
>> has <span class="<%= accessLevel.toLowerCase() %>"><%= accessLevel
>> %></span>
>> access to this application ( appId: <%= appId %> )
>> On 2010/08/19 23:09:01, kurrik.chromium wrote:
>> > Ignoring the wraps here because it's pretty hard to make HTML conform to
>> 80
>> > character widths
>>
>
>  Done.
>>
>
>  http://codereview.chromium.org/3122030/diff/1/5#newcode192
>> chrome/common/extensions/docs/examples/apps/hello-php/index.php:192: //
>> Simple
>> JavaScript Templating
>> On 2010/08/19 23:09:01, kurrik.chromium wrote:
>> > Put a copy of this in the NOTICE file
>>
>
>  Done.
>>
>
>  http://codereview.chromium.org/3122030/diff/1/5#newcode249
>> chrome/common/extensions/docs/examples/apps/hello-php/index.php:249: var
>> url =
>> [form.action, '&user_id=', encodeURIComponent(userId)].join('');
>> On 2010/08/19 23:09:01, kurrik.chromium wrote:
>> > You may want to consider fixing this wrap though, and the next one.
>>
>
>  Done.
>>
>
>
>
> http://codereview.chromium.org/3122030/show
>



-- 
Eric Bidelman
Developer Relations - Google
e.bidelman@google.com