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

Use yiisoft/yii-console with optional. #178

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

terabytesoftw
Copy link
Member

Q A
Is bugfix?
New feature? ✔️
Breaks BC?

Copy link

what-the-diff bot commented Nov 21, 2023

PR Summary

  • Update to README.md for clearer usage instructions

    • Instructions for usage with Yii Console and Symfony Console were added. Adding these equips users with more ways to use the product.
    • An actionable code example for configuring a logger with yiisoft/log-target-file was provided, making it easier for users to set up logging functionalities.
    • Instructions for adding aliases for the logger were given to allow users to use more convenient shortcuts for their tasks.
  • Addition new helper files

    • Three new files were added (bin/definitions.php, bin/queue, bin/queue.bat). These files are likely aimed at expanding the functionality or streamlining the usage of the product.
  • Modification of composer.json for enhanced package setup

    • "yiisoft/config" package was added to the dependencies. This aids in improved configuration management of the project
    • "bin/queue" was included in the "bin" section. This implies the addition of new command-line functionalities.
  • Updates to src/Command/ListenCommand.php and src/Command/RunCommand.php for consistent return values

    • Rather than using a symbolic constant (ExitCode::OK), the execute method in both files now consistently return 0 to signify success. This makes the code more straightforward to understand and follow.

@terabytesoftw terabytesoftw marked this pull request as ready for review November 22, 2023 10:37
Copy link

codecov bot commented Nov 22, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (eba0486) 94.31% compared to head (7910e61) 94.31%.

Additional details and impacted files
@@            Coverage Diff            @@
##             master     #178   +/-   ##
=========================================
  Coverage     94.31%   94.31%           
  Complexity      340      340           
=========================================
  Files            40       40           
  Lines           915      915           
=========================================
  Hits            863      863           
  Misses           52       52           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

composer.json Outdated Show resolved Hide resolved
@@ -58,6 +58,9 @@
"Yiisoft\\Yii\\Queue\\Tests\\": "tests"
}
},
"bin": [
"bin/queue"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"bin/queue"
"bin/yii-queue"

It's global space, I suggest use prefix.

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea when removing the console is to rename the package to yiisoft/queue, so queue makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

I suggest rename utility only. Because it is global namespace

bin/queue.bat Outdated Show resolved Hide resolved
bin/queue Outdated
use Yiisoft\Di\Container;
use Yiisoft\Di\ContainerConfig;

require_once './vendor/autoload.php';
Copy link
Member

Choose a reason for hiding this comment

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

Could there be a problem if run utility from different places?
May be better use absoulte path via __DIR__ (see yii2)?

Copy link
Member Author

@terabytesoftw terabytesoftw Nov 22, 2023

Choose a reason for hiding this comment

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

It is not necessary when using ./ it works well for commands, whether you install the package in an app or clone it.

Copy link
Member

Choose a reason for hiding this comment

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

I try it, and this do not work correclty.

Copy link
Member Author

Choose a reason for hiding this comment

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

As it fails, to reproduce it, since I use it in the app, and in the cloned directory, and it works correctly.

Copy link
Member

Choose a reason for hiding this comment

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

image

Copy link
Member Author

Choose a reason for hiding this comment

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

But you are inside the config directory, you must be root :)

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

Copy link
Member

Choose a reason for hiding this comment

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

Don't work. Add test.

Copy link
Member Author

Choose a reason for hiding this comment

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

image

image

It is a simple script, it works from the app's main directory, and from its own directory.

Copy link
Member

Choose a reason for hiding this comment

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

Utlitiy should work from any directory. Just use absolute path.

bin/queue Show resolved Hide resolved
bin/queue Outdated
use Yiisoft\Di\Container;
use Yiisoft\Di\ContainerConfig;

$autoload = $GLOBALS['_composer_autoload_path'] ?? './vendor/autoload.php';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$autoload = $GLOBALS['_composer_autoload_path'] ?? './vendor/autoload.php';
$autoload = $_composer_autoload_path ?? dirname(__DIR__, 3) . '/vendor/autoload.php';

https://getcomposer.org/doc/articles/vendor-binaries.md#finding-the-composer-autoloader-from-a-binary

bin/queue Outdated
require_once $autoload;

/** @var array $definitions */
$definitions = require_once 'definitions.php';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$definitions = require_once 'definitions.php';
$definitions = require_once __DIR__ . 'definitions.php';

Copy link
Member Author

@terabytesoftw terabytesoftw Nov 23, 2023

Choose a reason for hiding this comment

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

If you use the absolute path, when you copy the definitions to the ROOT Directory of the app, not work.

Copy link
Member

Choose a reason for hiding this comment

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

Not need copy definitions.

Copy link
Member Author

@terabytesoftw terabytesoftw Nov 23, 2023

Choose a reason for hiding this comment

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

If necessary, suppose that i have a different configuration than the predetermined, for example, i used monolog, when updating new version will lose my changes, that is fine, only when your configuration is equal to the predetermine

README.md Outdated

Just install `yiisoft/yii-console` package and you are ready to go.

## Usage with Symfony Console
Copy link
Member

Choose a reason for hiding this comment

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

For use with Symfony Console we should add commands to Symfony Console and do not use files from bin.

Copy link
Member Author

Choose a reason for hiding this comment

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

https://symfony.com/doc/current/components/console.html#creating-a-console-application Well in the documents it clearly says that you must register a script and add the commands, it is what is done in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

We should add instruction for use commands with symfony console that alreasy used in user app.

Copy link
Member Author

@terabytesoftw terabytesoftw Nov 23, 2023

Choose a reason for hiding this comment

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

I have to investigate it, i suggest merging this PR, and do it in another.

Copy link
Member Author

Choose a reason for hiding this comment

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

https://symfony.com/doc/current/console.html#registering-the-command According to this they are already registered, and the Symfony console should recognize them.

README.md Outdated Show resolved Hide resolved
@terabytesoftw
Copy link
Member Author

yiisoft/actions#66 fix mutation actions.

@@ -37,6 +37,48 @@ or add

to the `require` section of your `composer.json` file.

## Usage with Queue utility

1. Copy configuration file `./vendor/yiisoft/yii-queue/bin/QueueContainer.php` to `root` folder of your project.
Copy link
Member

Choose a reason for hiding this comment

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

Suggest require the user to create yii-queue-params.php in root directory with queue params only and don't require copy full container configuration. It's more cleary...

@@ -0,0 +1,83 @@
<?php
Copy link
Contributor

Choose a reason for hiding this comment

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

Emm, why do we need it?
Isn't it better to use all users' configs instead?

@samdark
Copy link
Member

samdark commented Dec 25, 2023

I think we need to adjust it the same way as yiisoft/db-migration@e0e4e49

@vjik vjik added the status:under development Someone is working on a pull request. label Jan 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:under development Someone is working on a pull request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants