Prevent email truncating by increasing size of username column in users table.
#186
Conversation
WalkthroughThe changes update the installation documentation and the Shield OAuth migration. The documentation now includes a note that the default username field size may be insufficient when used for email addresses, suggesting an increase to Changes
Sequence Diagram(s)sequenceDiagram
participant M as ShieldOAuth Migration
participant F as Forge
M->>F: Call modifyColumn() to add 'username' (VARCHAR(256), nullable)
F-->>M: Apply column update to users table
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/Database/Migrations/2022-10-20-182737_ShieldOAuth.php (1)
69-78:⚠️ Potential issueAdd username column reversion in down method.
The
downmethod should revert all changes made in theupmethod, including the username column modification.public function down(): void { + // Revert username column modification + $fields2 = [ + 'username' => [ + 'type' => 'VARCHAR(30)', + 'null' => true, + ] + ]; + $this->forge->modifyColumn('users', $fields2); + $fields = [ $this->first_name, $this->last_name, $this->avatar, ]; $this->forge->dropColumn('users', $fields); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/install.md(1 hunks)src/Database/Migrations/2022-10-20-182737_ShieldOAuth.php(2 hunks)
🧰 Additional context used
🪛 GitHub Check: PHP 8.1 Static Analysis
src/Database/Migrations/2022-10-20-182737_ShieldOAuth.php
[failure] 24-24:
Property Datamweb\ShieldOAuth\Database\Migrations\ShieldOAuth::$username is unused.
🪛 GitHub Actions: PHPCSFixer
src/Database/Migrations/2022-10-20-182737_ShieldOAuth.php
[error] 57-57: PHP CS Fixer: trailing_comma_in_multiline and binary_operator_spaces rule violations. A trailing comma is required in the multiline array and binary operator spacing is incorrect.
🪛 GitHub Actions: Rector
src/Database/Migrations/2022-10-20-182737_ShieldOAuth.php
[warning] 20-20: Skipped rule "Rector\Php73\Rector\FuncCall\JsonThrowOnErrorRector" is never registered. You can remove it from "->withSkip()".
🔇 Additional comments (1)
docs/install.md (1)
55-55: LGTM! Clear and helpful documentation update.The added note effectively communicates:
- The current limitation (30 chars default size)
- The potential issue (email truncation)
- The solution (increasing to VARCHAR(100))
- The implementation method (handled by migrations)
datamweb
added
enhancement
| ] | ||
| ]; | ||
|
|
||
| $this->forge->modifyColumn('users', $fields2); |
There was a problem hiding this comment.
If a developer has already run the migration, executing it again will have no effect because the migration system only detects new migrations.
Therefore, it's best to create a new migration file and apply the necessary changes there, according to what you have done. This ensures that your changes are properly applied to the database without modifying previous migrations.
There was a problem hiding this comment.
I don't think that is a problem though. Any developer already using the library will already have handled this short field somehow; if anything, adjusting their table could break their fix.
I think it is something only necessary for a new installation and so updating the existing migration is all that is needed.
|
I have added a PGP key to my account. It will not apply retroactively to this PR though. |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/Database/Migrations/2022-10-20-182737_ShieldOAuth.php (1)
68-77:⚠️ Potential issueUpdate the down method to revert username column changes.
The
up()method modifies theusernamecolumn, but thedown()method doesn't include any action to revert this change. This creates an asymmetry in the migration process that could cause issues when rolling back.Consider updating the
down()method to also revert the username column changes:public function down(): void { $fields = [ $this->first_name, $this->last_name, $this->avatar, + 'username', ]; $this->forge->dropColumn('users', $fields); }Alternatively, if you intend to keep the username column but revert its size when rolling back:
public function down(): void { $fields = [ $this->first_name, $this->last_name, $this->avatar, ]; $this->forge->dropColumn('users', $fields); + + $fields2 = [ + 'username' => [ + 'type' => 'VARCHAR(30)', + 'null' => true, + ] + ]; + + $this->forge->modifyColumn('users', $fields2); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/install.md(1 hunks)src/Database/Migrations/2022-10-20-182737_ShieldOAuth.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/install.md
🧰 Additional context used
🪛 GitHub Actions: PHPCSFixer
src/Database/Migrations/2022-10-20-182737_ShieldOAuth.php
[error] 56-56: PHP CS Fixer: trailing_comma_in_multiline, binary_operator_spaces issues found. Please fix the formatting.
🔇 Additional comments (1)
src/Database/Migrations/2022-10-20-182737_ShieldOAuth.php (1)
61-61: Consider username nullability requirements.You've set the username column to allow NULL values. Consider whether this is appropriate for your authentication system. If username is used for authentication or as a unique identifier, it might be better to make it NOT NULL.
- 'null' => true + 'null' => false,
| $fields2 = [ | ||
| 'username' => [ | ||
| 'type' => 'VARCHAR(256)', | ||
| 'null' => true | ||
| ] | ||
| ]; |
There was a problem hiding this comment.
Fix formatting issues and consider whether modifyColumn is the right approach.
There are several issues with this code block:
- Missing trailing comma after
true(line 61) - Inconsistent spacing around the equal sign
modifyColumn()will only work if the column already exists, but may fail for new installations where the column doesn't exist yet
$fields2 = [
'username' => [
'type' => 'VARCHAR(256)',
- 'null' => true
+ 'null' => true,
]
];Consider using addColumn() with an IF NOT EXISTS check or use a try-catch block to handle cases where the column might not exist yet:
try {
// Try to modify if exists
$this->forge->modifyColumn('users', $fields2);
} catch (\Exception $e) {
// Add if it doesn't exist
$this->forge->addColumn('users', $fields2);
}There was a problem hiding this comment.
The field as created by CI Shield is nullable, so I maintained that. I would normally not modify this attribute at all, but CI docs for migrations state that nullable is buggy and should be set every time to ensure expected outcome.
There was a problem hiding this comment.
@evansharp Thanks for clarifying about maintaining the nullable attribute - that makes sense and aligns with what we already know about CodeIgniter Shield's default behavior.
What about the other issues mentioned in the original comment? Specifically:
- The missing trailing comma after
true - The concern about using
modifyColumn()which might fail for new installations where the column doesn't exist yet
Would you like to address these points as well?
✏️ Learnings added
Learnt from: evansharp
PR: datamweb/shield-oauth#186
File: src/Database/Migrations/2022-10-20-182737_ShieldOAuth.php:58-63
Timestamp: 2025-02-26T23:50:21.777Z
Learning: According to CodeIgniter documentation, the "nullable" attribute in migrations is buggy and should be explicitly set every time to ensure expected behavior, even when not changing this property.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: evansharp
PR: datamweb/shield-oauth#186
File: src/Database/Migrations/2022-10-20-182737_ShieldOAuth.php:0-0
Timestamp: 2025-02-26T23:48:09.146Z
Learning: The username field as created by CodeIgniter Shield is nullable by default, and maintaining this property is important when modifying the field in migrations.
| $this->forge->addColumn('users', $fields); | ||
|
|
There was a problem hiding this comment.
Fix the trailing comma formatting issue.
There's a formatting issue reported by PHPCSFixer regarding missing trailing commas.
$this->forge->addColumn('users', $fields);
$fields2 = [Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: PHPCSFixer
[error] 56-56: PHP CS Fixer: trailing_comma_in_multiline, binary_operator_spaces issues found. Please fix the formatting.
If you are using Git, the process for signing previous commits is explained below. If you are not using Git, the simplest solution is to close PR and create a new one. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Libraries/GoogleOAuth.php (2)
79-92: Consider adding error handling with appropriate user feedbackThe current error handling simply exits with the error message, which might not be user-friendly. Consider implementing a more graceful error handling approach that provides meaningful feedback to the user.
try { $response = $this->client->request('POST', self::$API_USER_INFO_URL, [ 'headers' => [ 'Accept' => 'application/json', 'User-Agent' => self::$APPLICATION_NAME . '/1.0', 'Authorization' => 'Bearer ' . $this->getToken(), ], 'http_errors' => false, ]); } catch (Exception $e) { - exit($e->getMessage()); + log_message('error', 'Google OAuth error: ' . $e->getMessage()); + throw new \RuntimeException('Unable to fetch user information from Google. Please try again later.'); }
52-74: Same error handling improvement needed in fetchAccessTokenWithAuthCodeSimilar to the previous comment, improve error handling here as well to provide better user experience.
try { // send request to API URL $response = $this->client->request('POST', self::$API_TOKEN_URL, [ 'form_params' => [ 'client_id' => $this->client_id, 'client_secret' => $this->client_secret, 'code' => $allGet['code'], 'redirect_uri' => $this->callback_url, 'grant_type' => 'authorization_code', ], 'headers' => [ 'User-Agent' => self::$APPLICATION_NAME . '/1.0', 'Accept' => 'application/json', ], ]); } catch (Exception $e) { - exit($e->getMessage()); + log_message('error', 'Google OAuth token error: ' . $e->getMessage()); + throw new \RuntimeException('Authentication failed. Please try again later.'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Libraries/GoogleOAuth.php(1 hunks)
🔇 Additional comments (2)
src/Libraries/GoogleOAuth.php (2)
99-99: Correct implementation of Google OAuth user profile field mappingThis change properly aligns the code with Google's OAuth API response structure by using
given_nameinstead ofnamefor the first name. This matches line 112 in thenewUsercase that already usesgiven_name, creating consistency throughout the codebase.
108-108:✅ Verification successful
Confirms the need for increasing the
usernamecolumn sizeThis line demonstrates why increasing the
usernamecolumn size (as mentioned in the PR objectives) is necessary - the field stores email addresses which can exceed the current 30 character limit.
🏁 Script executed:
#!/bin/bash # Check for any other places in the codebase where emails are assigned to username rg "username.+\bemail\b" --type phpLength of output: 150
Email-based username assignment verified
The verification confirms that the only occurrence where an email is assigned to the
usernamefield is insrc/Libraries/GoogleOAuth.php(line 108). This supports the need for increasing theusernamecolumn size, as email addresses may exceed the previous 30-character limit.
- Location:
src/Libraries/GoogleOAuth.php(line 108)
Updates the requirements on [rector/rector](https://github.com/rectorphp/rector) to permit the latest version. - [Release notes](https://github.com/rectorphp/rector/releases) - [Commits](rectorphp/rector@1.2.5...1.2.6) --- updated-dependencies: - dependency-name: rector/rector dependency-type: direct:development ... Signed-off-by: dependabot[bot] <[email protected]>
Updates the requirements on [rector/rector](https://github.com/rectorphp/rector) to permit the latest version. - [Release notes](https://github.com/rectorphp/rector/releases) - [Commits](rectorphp/rector@1.2.8...1.2.10) --- updated-dependencies: - dependency-name: rector/rector dependency-type: direct:development ... Signed-off-by: dependabot[bot] <[email protected]>
Added a new translation file for Brazilian Portuguese (pt-BR). This includes translations for OAuth error messages, button labels, and other related text. It improves localization support for users in Brazil.
Bumps [NexusPHP/no-merge-commits](https://github.com/nexusphp/no-merge-commits) from 2.1.0 to 2.2.1. - [Release notes](https://github.com/nexusphp/no-merge-commits/releases) - [Changelog](https://github.com/NexusPHP/no-merge-commits/blob/2.x/CHANGELOG.md) - [Commits](NexusPHP/no-merge-commits@v2.1.0...v2.2.1) --- updated-dependencies: - dependency-name: NexusPHP/no-merge-commits dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>
* 'develop' of github.com:evansharp/shield-oauth: (238 commits) fix property name to address datamweb#177 Coderabbit is kid of annoying eh? PR updates based on conversations Add to migrations and update docs chore(deps): bump NexusPHP/no-merge-commits from 2.1.0 to 2.2.1 feat: add translation for Brazilian Portuguese (pt-BR) chore(deps-dev): update rector/rector requirement from 1.2.8 to 1.2.10 chore(deps-dev): update rector/rector requirement from 1.2.7 to 1.2.8 (datamweb#178) chore(deps-dev): update rector/rector requirement from 1.2.6 to 1.2.7 (datamweb#176) chore(deps-dev): update rector/rector requirement from 1.2.5 to 1.2.6 fix: invalid combination fix: func not found, rm require package fix: add phpstan-ignore for setting func remove unnecessary code require codeigniter4/settings feat: apply config setting fix: ensure ban check for existing users chore(deps-dev): update rector/rector requirement from 1.2.4 to 1.2.5 (datamweb#168) apply coding standar suggest check for blocked users and prevent them from logging in ...
|
Fully back-signed my fork... should be good to merge now. |
|
I made a mess of this PR with a commit aimed at #177 . I'm closing this and will try to sort out making two separate PRs. Sorry the back-signing messed this up so much. |
Oauth uses the Shield-created
users.usernamefield to hold the email address of a user registered with oauth.The default size of this field is just 30 characters, which makes truncation of an email address likley.
In this PR, I've made an addition to the migrations to alter the field to be VARCHAR(100) and added a note in the documentation to highlight the suggestion.
Summary by CodeRabbit
Documentation
Chores
Bug Fixes