add p:egg:normalize command - #2379
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Artisan command ChangesEgg File Normalization
Sequence DiagramsequenceDiagram
participant NormalizeEggCommand
participant EggImporterService
participant EggExporterService
participant Filesystem
NormalizeEggCommand->>Filesystem: read input file
NormalizeEggCommand->>EggImporterService: parse(content, format)
EggImporterService-->>NormalizeEggCommand: parsed egg array
NormalizeEggCommand->>NormalizeEggCommand: validate keys, upgrade metadata, normalize variables
NormalizeEggCommand->>NormalizeEggCommand: hasFileChanged (regenerate & compare)
NormalizeEggCommand->>EggExporterService: yamlExport(egg)
EggExporterService-->>NormalizeEggCommand: YAML string
NormalizeEggCommand->>Filesystem: write .yaml output
NormalizeEggCommand->>Filesystem: optionally delete original
Possibly Related PRs
🚥 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. 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 |
7 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@app/Console/Commands/Egg/NormalizeEggCommand.php`:
- Around line 36-40: When catching InvalidFileUploadException around the
$importer->parse($unparsed, $format) call in NormalizeEggCommand.php, don't
discard the original exception; either rethrow the caught
InvalidFileUploadException or throw a new Exception that wraps it as the
previous exception (pass the caught exception as the third parameter to
Exception) so the original stack trace and message are preserved for debugging;
update the catch block accordingly to include the original exception context.
- Around line 47-50: The deletion currently calls unlink($inputFile) without
checking for failure; update the block guarded by
$this->option('delete-original') (where $outputFile !== $inputFile) to attempt
unlink($inputFile), verify its boolean return, and if it fails call
$this->error(...) to inform the user (include file name and, optionally,
error_get_last()['message'] for detail); keep the success info message
("Deleting {$inputFile} as requested") only if unlink returns true and do not
suppress PHP warnings so failures can be reported.
- Around line 52-54: The file write in NormalizeEggCommand (around the export
logic using Yaml::dump and file_put_contents) doesn't check
file_put_contents()'s return value; update the export block in the
NormalizeEggCommand class/method to capture the return of
file_put_contents($outputFile, $yaml), verify it is not === false, and handle
failures by reporting an error via $this->error() and returning a non-zero exit
code (or throwing an exception) so the command indicates failure to the caller;
on success, continue to call $this->info() as before.
- Around line 24-26: Validate that the input file exists and is readable before
calling file_get_contents: check is_readable($inputFile) (or file_exists +
is_readable) right after $inputFile = $this->argument('file'), and if it fails
call $this->error("...") and return a non-zero status (e.g., return 1) from the
command (this is inside the NormalizeEggCommand::handle/execute method) so
downstream code using $unparsed and pathinfo() does not operate on false.
- Around line 42-45: The code assumes $eggArray['meta'] exists before setting
fields and calling self::replaceExtension; modify the NormalizeEggCommand logic
to validate and ensure the meta structure exists (e.g. check
is_array($eggArray['meta']) or isset($eggArray['meta']) and initialize it as an
array if missing) before assigning $eggArray['meta']['version'] =
Egg::EXPORT_VERSION and before calling
self::replaceExtension($eggArray['meta']['update_url']); keep using
Egg::EXPORT_VERSION and the existing replaceExtension method but guard all
accesses to $eggArray['meta'] to prevent undefined key errors.
- Around line 16-20: The class NormalizeEggCommand defines the command twice
(attribute #[Signature('p:egg:normalize {file} {--delete-original}')] and a
conflicting protected $signature = 'p:egg:normalize'; property); remove the
duplicate property-based signature (the protected $signature field) and rely on
the attribute-based Signature so the command arguments ({file} and
{--delete-original}) are parsed correctly by Laravel.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0ac80155-03d0-46cd-8f2b-83d7ed33d9ad
📒 Files selected for processing (3)
app/Console/Commands/Egg/NormalizeEggCommand.phpapp/Services/Eggs/Sharing/EggExporterService.phpapp/Services/Eggs/Sharing/EggImporterService.php
harryyoud
force-pushed
the
egg-normalize-command
branch
from
June 7, 2026 18:24
0963466 to
942f53c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/Console/Commands/Egg/NormalizeEggCommand.php (1)
51-51:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard access to
update_urlkey.Line 51 accesses
$eggArray['meta']['update_url']without verifying the key exists. If an egg file is missing theupdate_urlfield, this will cause an undefined array key error.🛡️ Proposed fix
$eggArray['meta']['version'] = Egg::EXPORT_VERSION; -$eggArray['meta']['update_url'] = self::replaceExtension($eggArray['meta']['update_url']); +if (isset($eggArray['meta']['update_url'])) { + $eggArray['meta']['update_url'] = self::replaceExtension($eggArray['meta']['update_url']); +} $eggArray['exported_at'] = Carbon::now()->toAtomString();🤖 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 `@app/Console/Commands/Egg/NormalizeEggCommand.php` at line 51, Guard access to the update_url key before calling replaceExtension: check that $eggArray contains a 'meta' array and that 'update_url' exists (e.g. using isset or array_key_exists) before doing the assignment; update the code in NormalizeEggCommand (the method that performs this transformation where $eggArray['meta']['update_url'] is referenced) to only call self::replaceExtension($eggArray['meta']['update_url']) when the key is present, leaving $eggArray unchanged otherwise.
🧹 Nitpick comments (1)
app/Console/Commands/Egg/NormalizeEggCommand.php (1)
25-29: ⚡ Quick winConsider checking file existence before reading.
While the code correctly handles the
falsereturn fromfile_get_contents, it will still emit a PHP warning if the file doesn't exist. Adding an explicitfile_exists()oris_readable()check before line 25 would provide clearer error messaging and avoid warnings in logs.♻️ Proposed improvement
$inputFile = $this->argument('file'); + +if (!file_exists($inputFile)) { + $this->error("File not found: {$inputFile}"); + return Command::FAILURE; +} + $unparsed = file_get_contents($inputFile); if ($unparsed === false) { $this->error("Failed to read file: {$inputFile}");🤖 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 `@app/Console/Commands/Egg/NormalizeEggCommand.php` around lines 25 - 29, Check for file existence/readability before calling file_get_contents on $inputFile: use is_readable($inputFile) (or file_exists) to detect missing/unreadable files and call $this->error with a clear message and return Command::FAILURE instead of allowing file_get_contents($inputFile) to emit a PHP warning; update the block around file_get_contents, $unparsed and the early return (referencing file_get_contents, $inputFile, $unparsed and Command::FAILURE) to perform this guard check first.
🤖 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 `@app/Console/Commands/Egg/NormalizeEggCommand.php`:
- Around line 70-74: The file write error branch currently logs the error but
still returns Command::SUCCESS; modify NormalizeEggCommand (the code around the
file_put_contents call) so that if file_put_contents($outputFile, $yaml) ===
false you both call $this->error("Failed to write output file: {$outputFile}")
and return Command::FAILURE (instead of falling through to Command::SUCCESS) so
the command exits with a failure status; ensure the normal successful path still
returns Command::SUCCESS.
- Around line 54-57: In NormalizeEggCommand, guard access to
$eggArray['variables'] and each variable's keys: first check that
$eggArray['variables'] exists and is an array before iterating, then inside the
loop verify keys exist (e.g., isset($var['field_type']) before unset and
isset($var['rules']) before processing) and normalize missing rules to an empty
array or string before calling explode; update the loop that iterates
$eggArray['variables'] to only run when it's an array and to handle absent
$var['rules'] safely so no undefined array key notices occur.
---
Duplicate comments:
In `@app/Console/Commands/Egg/NormalizeEggCommand.php`:
- Line 51: Guard access to the update_url key before calling replaceExtension:
check that $eggArray contains a 'meta' array and that 'update_url' exists (e.g.
using isset or array_key_exists) before doing the assignment; update the code in
NormalizeEggCommand (the method that performs this transformation where
$eggArray['meta']['update_url'] is referenced) to only call
self::replaceExtension($eggArray['meta']['update_url']) when the key is present,
leaving $eggArray unchanged otherwise.
---
Nitpick comments:
In `@app/Console/Commands/Egg/NormalizeEggCommand.php`:
- Around line 25-29: Check for file existence/readability before calling
file_get_contents on $inputFile: use is_readable($inputFile) (or file_exists) to
detect missing/unreadable files and call $this->error with a clear message and
return Command::FAILURE instead of allowing file_get_contents($inputFile) to
emit a PHP warning; update the block around file_get_contents, $unparsed and the
early return (referencing file_get_contents, $inputFile, $unparsed and
Command::FAILURE) to perform this guard check first.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 72b20dcc-98e7-4c72-a488-5f29009b955a
📒 Files selected for processing (3)
app/Console/Commands/Egg/NormalizeEggCommand.phpapp/Services/Eggs/Sharing/EggExporterService.phpapp/Services/Eggs/Sharing/EggImporterService.php
🚧 Files skipped from review as they are similar to previous changes (2)
- app/Services/Eggs/Sharing/EggExporterService.php
- app/Services/Eggs/Sharing/EggImporterService.php
harryyoud
force-pushed
the
egg-normalize-command
branch
from
June 7, 2026 18:35
942f53c to
2ced2a9
Compare
Contributor
Author
|
I disagree with CodeRabbitAI above regarding this comment: ====================== 25-29: ⚡ Quick win Consider checking file existence before reading. While the code correctly handles the false return from file_get_contents, it will still emit a PHP warning if the file doesn't exist. Adding an explicit file_exists() or is_readable() check before line 25 would provide clearer error messaging and avoid warnings in logs. ====================== I think this would just introduce a TOCTOU, and it's easier to ask for forgiveness than permission in this scenario. We don't need elegant or graceful error handling in console as long as we are safe and bail out on error and leave a sensible error message. |
Member
|
I don't get why this command is needed. |
Contributor
Author
|
When the panel checks for updates to the egg, it compares the local egg as json to the remote egg as json. If the remote egg has an older version (e.g. PTDL_V2) or array items in a different order, it will appear out of date every single time. Right now, the Minecraft eggs repo as an example has a large number of eggs in this way - and so when the check egg cron job is fixed, many of these eggs will always show that they need an update. I thought to enable consistency, I created this command to enable automation of upgrading/normalizing egg files in the repos. This could potentially be used for CI on the egg repos to ensure the files are consistently formatted and don't constantly trigger an out of date state on the panel. See pelican-eggs/minecraft#133 as an example of this in action |
Member
|
Thanks for the contribution! After re-reading it, I believe it would be good to have this. However, can you make the following adjustments first?
|
harryyoud
force-pushed
the
egg-normalize-command
branch
from
June 14, 2026 01:15
2ced2a9 to
a1f3571
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@app/Console/Commands/Egg/NormalizeEggCommand.php`:
- Around line 57-72: The validation in NormalizeEggCommand.php only checks for
key presence using array_key_exists but does not validate the actual types of
the values. Before calling replaceExtension() on the meta.update_url value, add
additional guard clauses to verify that meta itself is an array and that
meta.update_url is specifically a string. If either condition fails, log an
appropriate error message and return Command::FAILURE, similar to the existing
validation pattern in the block, to prevent TypeError when replaceExtension() is
invoked with a non-string argument.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 815f4e1d-3125-4e0f-a8d8-76e7641ff885
📒 Files selected for processing (6)
app/Console/Commands/Egg/NormalizeEggCommand.phpapp/Services/Eggs/Sharing/EggExporterService.phpapp/Services/Eggs/Sharing/EggImporterService.phptests/Unit/Console/Commands/Egg/NormalizerEggCommandTest.phptests/Unit/Console/Commands/Egg/egg-plcnv3-example.yamltests/Unit/Console/Commands/Egg/egg-ptdlv2.json
✅ Files skipped from review due to trivial changes (2)
- tests/Unit/Console/Commands/Egg/egg-plcnv3-example.yaml
- tests/Unit/Console/Commands/Egg/egg-ptdlv2.json
🚧 Files skipped from review as they are similar to previous changes (2)
- app/Services/Eggs/Sharing/EggImporterService.php
- app/Services/Eggs/Sharing/EggExporterService.php
harryyoud
force-pushed
the
egg-normalize-command
branch
from
June 14, 2026 01:28
a1f3571 to
1501bf5
Compare
used to upgrade and normalize egg files for programmatic upgrading of eggs
harryyoud
force-pushed
the
egg-normalize-command
branch
from
June 14, 2026 01:30
1501bf5 to
b0cd795
Compare
lancepioch
approved these changes
Jun 18, 2026
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
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.
used to upgrade and normalize egg files for programmatic upgrading of eggs