Skip to content

Fix importer scalar option path resolution#59

Open
samdark wants to merge 1 commit into
masterfrom
fix-importer-option-path-resolution
Open

Fix importer scalar option path resolution#59
samdark wants to merge 1 commit into
masterfrom
fix-importer-option-path-resolution

Conversation

@samdark

@samdark samdark commented Jun 12, 2026

Copy link
Copy Markdown
Member

Summary

  • add an explicit ImporterOption::resolvePath flag for filesystem path options
  • keep scalar importer options such as Telegram ignore_message_ids literal
  • document path resolution behavior for importer authors and Telegram users

Tests

  • make test -- --filter ImportCommandTest
  • make psalm
  • make test

Summary by CodeRabbit

  • New Features

    • Added --ignore_message_ids option for Telegram imports to selectively skip specific message IDs from being imported.
  • Bug Fixes

    • Fixed incorrect path resolution behavior that treated scalar values like IDs and tokens as filesystem paths.
  • Documentation

    • Updated Telegram importer and import options documentation with clarifications on distinguishing filesystem paths from scalar values.
  • Tests

    • Added test coverage for scalar import option handling during import operations.

Copilot AI review requested due to automatic review settings June 12, 2026 22:19
@coderabbitai

coderabbitai Bot commented Jun 12, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 43e1be2e-faa3-4a90-babd-cd4c2c1dc40d

📥 Commits

Reviewing files that changed from the base of the PR and between 315ad8d and d46725b.

📒 Files selected for processing (6)
  • docs/commands.md
  • docs/importing-content.md
  • src/Console/ImportCommand.php
  • src/Import/ImporterOption.php
  • src/Import/Telegram/TelegramContentImporter.php
  • tests/Unit/Console/ImportCommandTest.php

📝 Walkthrough

Walkthrough

The PR adds a resolvePath boolean flag to ImporterOption that controls whether importer option values are resolved as filesystem paths. ImportCommand now checks this flag before path resolution, TelegramContentImporter enables it for directory options, and the behavior is documented and tested.

Changes

Path Resolution Control for Importer Options

Layer / File(s) Summary
Path resolution control contract and logic
src/Import/ImporterOption.php, src/Console/ImportCommand.php
ImporterOption gains a public resolvePath bool property (defaulting false) with documentation clarifying it should be enabled only for filesystem-path options. ImportCommand::resolveImporterOptions() now checks $option->resolvePath in addition to null/empty checks before calling resolvePath().
TelegramContentImporter path resolution configuration
src/Import/Telegram/TelegramContentImporter.php
The directory option is marked with resolvePath: true to enable path resolution for the Telegram export directory while other scalar options like ignore_message_ids remain unresolved.
Documentation and test coverage
docs/importing-content.md, docs/commands.md, tests/Unit/Console/ImportCommandTest.php
Docs explain when to enable resolvePath for filesystem paths vs. keeping it disabled for scalar values (IDs, URLs, tokens, limits). New unit test testDoesNotResolveScalarImporterOptionsAsPaths validates that scalar options are not treated as filesystem paths during import.

🎯 2 (Simple) | ⏱️ ~12 minutes

🐰 Path flags now guide the way,
Scalar values skip the fray,
Directories find their home true,
With resolvePath set just right for you!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Fix importer scalar option path resolution' directly and accurately summarizes the main change: preventing scalar importer options from being incorrectly resolved as filesystem paths.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-importer-option-path-resolution
⚔️ Resolve merge conflicts
  • Resolve merge conflict in branch fix-importer-option-path-resolution

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refines how yii import resolves importer option values by making filesystem path resolution an explicit opt-in, preventing scalar options (like Telegram’s ignore_message_ids) from being incorrectly treated as paths.

Changes:

  • Added ImporterOption::$resolvePath to explicitly mark which importer options should be resolved relative to the project root.
  • Updated ImportCommand to resolve paths only for options with resolvePath: true, and set this for Telegram’s --directory.
  • Added a regression test and documentation updates describing the intended behavior.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tests/Unit/Console/ImportCommandTest.php Adds a test ensuring scalar importer options aren’t path-resolved.
src/Import/Telegram/TelegramContentImporter.php Marks directory option as resolvePath: true while keeping scalar options literal.
src/Import/ImporterOption.php Introduces the new resolvePath flag on importer options.
src/Console/ImportCommand.php Applies path resolution conditionally based on ImporterOption::$resolvePath.
docs/importing-content.md Documents how importer authors should use resolvePath.
docs/commands.md Documents Telegram importer behavior for scalar options.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread docs/importing-content.md
```

- **`name`** — option name, used as `--name` on the CLI and as key in the `$options` array.
- **`description`** — help text shown in `yii import --help`.
Comment thread docs/commands.md
Comment on lines 169 to +170
- `--directory` — path to the Telegram export directory containing `result.json` (required). Absolute or relative to project root.
- `--ignore_message_ids` — comma-separated message IDs to skip. This is treated as a literal scalar value, not a path.
Comment on lines 10 to +12
* Each importer returns a list of these from {@see ContentImporterInterface::options()}.
* The `yii import` command registers them as CLI options and passes resolved values to the importer.
* Set `$resolvePath` to true only for options that represent filesystem paths.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants