Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add isCli removal chapter #345

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add isCli removal chapter #345

wants to merge 1 commit into from

Conversation

laoneo
Copy link
Member

@laoneo laoneo commented Dec 12, 2024

User description

for joomla/joomla-cms#44611


PR Type

Documentation


Description

  • Added documentation explaining the removal of the deprecated isCli function from application classes
  • Included migration guide with code examples showing how to replace $app->isCli() checks with $app instanceof ConsoleApplication
  • Referenced the original PR (#44611) where the change was implemented

Changes walkthrough 📝

Relevant files
Documentation
removed-backward-incompatibility.md
Documentation for isCli function removal and migration guide

migrations/54-60/removed-backward-incompatibility.md

  • Added documentation for the removal of isCli function from application
    classes
  • Provided code examples showing how to migrate from old isCli() usage
    to checking for ConsoleApplication instance
  • +17/-0   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link
    Contributor

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Documentation Clarity
    The migration guide should mention potential impact on extensions and recommend testing CLI-dependent functionality after migration

    Copy link
    Contributor

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Improve code readability and maintainability by properly importing classes instead of using fully qualified names inline

    Add a note about using instanceof with the fully qualified class name to prevent
    potential namespace issues and ensure the code works even if the class is not
    imported.

    migrations/54-60/removed-backward-incompatibility.md [80]

    -if ($app instanceof \Joomla\CMS\Application\ConsoleApplication) {
    +use Joomla\CMS\Application\ConsoleApplication;
     
    +if ($app instanceof ConsoleApplication) {
    +
    • Apply this suggestion
    Suggestion importance[1-10]: 2

    Why: While using imports can improve readability, in a migration guide it's often clearer to show the fully qualified class name directly in the example to avoid any ambiguity. The suggestion would make the example less self-contained.

    2

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant