Support BlenderID OAuth2 login in phabricator.
Support BlenderID badge system
Details
Details
Diff Detail
Diff Detail
Event Timeline
Comment Actions
i couldnt commit files inside /src/extensions/ as its inside the .gitignore, so i moved them out
Comment Actions
The blender-logo.png file doesn't want to show itself here; just wondering if PNG is the best choice here. Is it used in one place with predictable size? Otherwise SVG might be better.
| src/extensions/blender-id/PhabricatorBlenderIDAuthProvider.php | ||
|---|---|---|
| 26 | I don't know the code style rules used in this project, but this line seems to be rather long. | |
| 117 | Again, I'm not sure about the code style rules for this project, but in most projects we prefer full English sentences (so starting with a capital letter, ending with a period, and having the proper punctuation) for comments. | |
| src/extensions/blender-id/PhutilBlenderIDAuthAdapter.php | ||
| 77 | There is no 'Bleder', and Blender doesn't require anything. 'Blender ID' is the proper name. | |
| 90 | Same disclaimer about code style rules, but generally we end files with a newline. Same for other files (like blender-id.css). | |
| src/extensions/blender-id/README.md | ||
| 2 | The sections below are probably steps to perform in order to use this extension. Explicitly mentioning this is a good idea. | |
| 14 | I don't think this README should express any preference to the name of the OAuth2 Application. | |
| 15 | typo | |
| 15 | If at all predictable, I would add an example here, f.e. https://sitename/oauth2/return or whatever is the url. This makes it easier to find the correct URL to configure. | |
| 26 | typo | |
Comment Actions
Updates of the code style to fit the rules.
The blender-logo.png is used only on the login page. It has only 2 possible sizes:
- For non retina displays 28px x 28px
- For retina displays 56px x 56px
| src/extensions/blender-id/PhabricatorBlenderIDAuthProvider.php | ||
|---|---|---|
| 14 | Ok, this is now a full sentence, but it has also lost information. Previously it was clear that the string returned was a CSS class, now that info is lost. Why not just write "CSS class of the Blender icon."? | |
| 20 | Try to keep such comments about the function. To me this comment reads like "In Paris you can see the Eiffel Tower"; something that can be true, but it's unclear how it relates to the code. "Returns the help text displayed at the Edit Auth Provider section." seems much clearer to me. | |
| 76 | Same as above. This comment makes a statement about what "we" want, but doesn't relate it to the function. "Adds the Blender ID URI to the Phabricator config array." reads better to me. | |
Comment Actions
@Sybren A. Stüvel (sybren) Thanks you so much for your comments and your patience. It is much appreciated. Sorry that it has be comments on my comments :) I am still adapting myself to such a big open source project like Blender. But if you bare with me, and i am sure i can be useful for Blender as i have a some experience in the web development.
| src/extensions/blender-id/PhabricatorBlenderIDAuthProvider.php | ||
|---|---|---|
| 26 | see here where the text is displayed. It is the same length than other Oauth provider in Phabricator.  | |
| 117 | OK! :) | |
| src/extensions/blender-id/PhutilBlenderIDAuthAdapter.php | ||
| 90 | Ok. I have misunderstood a previous comment from Sergey. All Phabricator class file seems to end with an empty line. | |
Comment Actions
Please update the patch description so that it clearly states what features are added, and also what is not included if something is purposely left out (like asynchronous downloading of avatars). If the patch description doesn't say what functionality is supposed to be there, it's also impossible to tell whether it's complete & doing what it should be doing.
| src/extensions/blender-id/PhabricatorBlenderIDAuthProvider.php | ||
|---|---|---|
| 26 | I'm talking about line length in the code, not the length of the string. | |
Comment Actions
I have corrected a bug in the Badge library when picking up the Blender ID provider settings.
| src/extensions/blender-id/adapter/PhutilBlenderIDAuthAdapter.php | ||
|---|---|---|
| 92 ↗ | (On Diff #19512) | try { |
| src/extensions/blender-id/badges/LibBlenderIDBadges.php | ||
| 32 ↗ | (On Diff #19512) | Spaces. Please follow Phabricator's style, and be more consistent in the own code. |
| 49 ↗ | (On Diff #19512) | empty() instead of counting actual number of elements. |
| 67 ↗ | (On Diff #19512) | Early output instead of extra level of indentation. |
Comment Actions
- Bot username can now be setup inside the Auth application settings.
- If not present, the Badges are created simply via its name.
- I have corrected a lot of code style.