fix(selection-plan): allow emoji in SubmissionPeriodDisclaimer#554
Open
romanetar wants to merge 1 commit into
Open
fix(selection-plan): allow emoji in SubmissionPeriodDisclaimer#554romanetar wants to merge 1 commit into
romanetar wants to merge 1 commit into
Conversation
Signed-off-by: romanetar <roman_ag@hotmail.com>
📝 WalkthroughWalkthroughDatabase configuration is upgraded from UTF-8 to UTF-8MB4 encoding across both model and write connections, a corresponding schema migration alters the SelectionPlan disclaimer column to match, and a regression test validates emoji characters persist correctly through the update API. ChangesUTF8MB4 Charset and Emoji Support
🎯 2 (Simple) | ⏱️ ~10 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-554/ This page is automatically updated on each push to this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
database/migrations/model/Version20260603184510.php (2)
27-27: 💤 Low valueCollation differs from database default.
The migration specifies
utf8mb4_general_ciwhile the database configuration (inconfig/database.php:12,65) defaults toutf8mb4_unicode_ci.
utf8mb4_general_ci: Faster, less accurate Unicode sortingutf8mb4_unicode_ci: Slower, more accurate international character sortingIf
SubmissionPeriodDisclaimeris primarily for display and not sorting/comparison,general_ciis fine. Otherwise, consider aligning with the database default.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@database/migrations/model/Version20260603184510.php` at line 27, The migration alters the SelectionPlan column SubmissionPeriodDisclaimer to use utf8mb4_general_ci which diverges from the app DB default utf8mb4_unicode_ci; update the ALTER TABLE statement in Version20260603184510 (the MODIFY for SubmissionPeriodDisclaimer) to use utf8mb4_unicode_ci to match config/database.php, or if you intentionally want faster general collation, add a short comment in the migration explaining why utf8mb4_general_ci is required so reviewers know the deviation is deliberate.
35-38: 💤 Low valueDocument why
down()migration is empty.The empty
down()method means there's no rollback path for this schema change. While this might be intentional (rolling back from utf8mb4 to utf8 would corrupt emoji, and TEXT to VARCHAR could truncate data), it should be documented.📝 Suggested documentation
public function down(Schema $schema): void { - + // Intentionally empty: rolling back from utf8mb4→utf8 would corrupt emoji characters, + // and TEXT→VARCHAR(255) could truncate existing data. }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@database/migrations/model/Version20260603184510.php` around lines 35 - 38, Add a brief doc comment above the down(Schema $schema): void method explaining why the migration is intentionally irreversible: note that the up() changes alter column types/charsets (e.g., conversion to utf8mb4 which would lose emoji if reverted to utf8, and TEXT-to-VARCHAR changes that could truncate data) and therefore no safe rollback is provided; reference the method name down and the corresponding up migration in the comment for future maintainers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@database/migrations/model/Version20260603184510.php`:
- Around line 26-29: The entity mapping for
SelectionPlan.SubmissionPeriodDisclaimer doesn't match the migration: update the
ORM mapping in app/Models/Foundation/Summit/SelectionPlan.php so the property
SubmissionPeriodDisclaimer is declared as type: 'text' and nullable: true (e.g.
change #[ORM\Column(name: 'SubmissionPeriodDisclaimer', type: 'string')] to
#[ORM\Column(name: 'SubmissionPeriodDisclaimer', type: 'text', nullable: true)]
and remove any length restriction), ensuring Doctrine metadata matches the ALTER
TABLE in Version20260603184510.php; after the change, run your schema validation
or tests to confirm no metadata/schema drift remains.
---
Nitpick comments:
In `@database/migrations/model/Version20260603184510.php`:
- Line 27: The migration alters the SelectionPlan column
SubmissionPeriodDisclaimer to use utf8mb4_general_ci which diverges from the app
DB default utf8mb4_unicode_ci; update the ALTER TABLE statement in
Version20260603184510 (the MODIFY for SubmissionPeriodDisclaimer) to use
utf8mb4_unicode_ci to match config/database.php, or if you intentionally want
faster general collation, add a short comment in the migration explaining why
utf8mb4_general_ci is required so reviewers know the deviation is deliberate.
- Around line 35-38: Add a brief doc comment above the down(Schema $schema):
void method explaining why the migration is intentionally irreversible: note
that the up() changes alter column types/charsets (e.g., conversion to utf8mb4
which would lose emoji if reverted to utf8, and TEXT-to-VARCHAR changes that
could truncate data) and therefore no safe rollback is provided; reference the
method name down and the corresponding up migration in the comment for future
maintainers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: def307de-a207-46bb-90f4-202c09c02e68
📒 Files selected for processing (3)
config/database.phpdatabase/migrations/model/Version20260603184510.phptests/oauth2/OAuth2SummitSelectionPlansApiTest.php
Comment on lines
+26
to
+29
| $sql = <<<SQL | ||
| ALTER TABLE `SelectionPlan` MODIFY SubmissionPeriodDisclaimer TEXT CHARACTER SET utf8mb4 COLLATE utf8mb4_general_ci NULL; | ||
| SQL; | ||
| $this->addSql($sql); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the ORM Column declaration for SubmissionPeriodDisclaimer
echo "=== Current entity mapping ==="
rg -n -A2 "SubmissionPeriodDisclaimer.*Column" --type=php app/Models/
echo ""
echo "=== Check if Doctrine validation catches this ==="
echo "Run: php artisan doctrine:schema:validate"Repository: OpenStackweb/summit-api
Length of output: 191
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Find SelectionPlan entity files ==="
fd -i "SelectionPlan\.php$" . || true
echo
echo "=== Search all occurrences of SubmissionPeriodDisclaimer ==="
rg -n "SubmissionPeriodDisclaimer" . || true
echo
echo "=== Show migration ALTER TABLE statement for SubmissionPeriodDisclaimer ==="
rg -n "ALTER TABLE `SelectionPlan`|SubmissionPeriodDisclaimer" database/migrations/model/Version20260603184510.php || trueRepository: OpenStackweb/summit-api
Length of output: 2490
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Entity mapping: app/Models/Foundation/Summit/SelectionPlan.php around SubmissionPeriodDisclaimer ==="
nl -ba app/Models/Foundation/Summit/SelectionPlan.php | sed -n '120,180p'
echo
echo "=== Migration file: database/migrations/model/Version20260603184510.php (up/down) ==="
nl -ba database/migrations/model/Version20260603184510.php | sed -n '1,120p'
echo
echo "=== Repo collation references (utf8mb4_unicode_ci / utf8mb4_general_ci) ==="
rg -n "utf8mb4_unicode_ci|utf8mb4_general_ci" . || trueRepository: OpenStackweb/summit-api
Length of output: 233
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Entity mapping around app/Models/Foundation/Summit/SelectionPlan.php (140-170) ==="
cat -n app/Models/Foundation/Summit/SelectionPlan.php | sed -n '130,175p'
echo
echo "=== Migration file around database/migrations/model/Version20260603184510.php (1-80) ==="
cat -n database/migrations/model/Version20260603184510.php | sed -n '1,120p'
echo
echo "=== Migration file: find the exact ALTER statement line(s) ==="
rg -n "ALTER TABLE `SelectionPlan`|SubmissionPeriodDisclaimer" database/migrations/model/Version20260603184510.php
echo
echo "=== Collation references in repo (utf8mb4_unicode_ci / utf8mb4_general_ci) ==="
rg -n "utf8mb4_unicode_ci|utf8mb4_general_ci" . || trueRepository: OpenStackweb/summit-api
Length of output: 4496
Fix Doctrine mapping for SubmissionPeriodDisclaimer (type + nullability)
- Migration
database/migrations/model/Version20260603184510.phpsetsSelectionPlan.SubmissionPeriodDisclaimertoTEXT ... NULL. - Entity
app/Models/Foundation/Summit/SelectionPlan.phpdeclares#[ORM\Column(name: 'SubmissionPeriodDisclaimer', type: 'string')](Doctrinestring⇒VARCHAR(255)by default) and does not mark it nullable—so ORM metadata won’t match the actual schema.
📝 Proposed fix for the entity mapping
- #[ORM\Column(name: 'SubmissionPeriodDisclaimer', type: 'string')]
+ #[ORM\Column(name: 'SubmissionPeriodDisclaimer', type: 'text', nullable: true)]
private $submission_period_disclaimer;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@database/migrations/model/Version20260603184510.php` around lines 26 - 29,
The entity mapping for SelectionPlan.SubmissionPeriodDisclaimer doesn't match
the migration: update the ORM mapping in
app/Models/Foundation/Summit/SelectionPlan.php so the property
SubmissionPeriodDisclaimer is declared as type: 'text' and nullable: true (e.g.
change #[ORM\Column(name: 'SubmissionPeriodDisclaimer', type: 'string')] to
#[ORM\Column(name: 'SubmissionPeriodDisclaimer', type: 'text', nullable: true)]
and remove any length restriction), ensuring Doctrine metadata matches the ALTER
TABLE in Version20260603184510.php; after the change, run your schema validation
or tests to confirm no metadata/schema drift remains.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ref https://app.clickup.com/t/86b99xjvv
Summary by CodeRabbit
Release Notes
New Features
Tests