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

Provide a usefull error when ask*() is not used wihtin a task() #1083

Merged
merged 3 commits into from
Mar 11, 2017

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Mar 10, 2017

Q A
Bug fix? No
New feature? Yes
BC breaks? No
Deprecations? No
Fixed tickets N/A

before this change a newcomer to this project sees errors like

PHP Fatal error:  Uncaught Error: Call to a member function getOutput() on boolean in /cluster/www/www/www/tools/deployer/src/functions.php:677
Stack trace:
#0 /cluster/www/www/www/tools/deployer/src/functions.php(685): Deployer\output()
#1 /cluster/www/www/www/tools/deployer/src/functions.php(567): Deployer\isQuiet()
#2 /deploy/kunzmann/stage/rocket/deploy.php(51): Deployer\ask('Zielstage (prod...')
#3 /cluster/www/www/www/tools/deployer/bin/dep(106): require('/deploy/kunzman...')
#4 /cluster/www/www/www/tools/deployer/bin/dep(107): {closure}()
#5 {main}
  thrown in /cluster/www/www/www/tools/deployer/src/functions.php on line 677

when using a ask() function within global scope instead of a task().

After this change the error turns into

PHP Fatal error:  Uncaught Deployer\Exception\Exception: 'Deployer\ask' can only be used within a 'task()'-function! in /cluster/www/www/www/tools/deployer/src/functions.php:757
Stack trace:
#0 /cluster/www/www/www/tools/deployer/src/functions.php(567): Deployer\requiresTaskContext('Deployer\\ask')
#1 /deploy/kunzmann/stage/rocket/deploy.php(51): Deployer\ask('Zielstage (prod...')
#2 /cluster/www/www/www/tools/deployer/bin/dep(106): require('/deploy/kunzman...')
#3 /cluster/www/www/www/tools/deployer/bin/dep(107): {closure}()
#4 {main}
  thrown in /cluster/www/www/www/tools/deployer/src/functions.php on line 757

based on a "discussion" in #371 (comment)

@staabm
Copy link
Contributor Author

staabm commented Mar 10, 2017

This small new function can obviously be used in more places but I thought this would be a good start.

@antonmedv
Copy link
Member

Cool. i think this is helpful.

@antonmedv
Copy link
Member

Please update changelog.

@staabm
Copy link
Contributor Author

staabm commented Mar 10, 2017

done.

* This method provides a usefull error to the end-user to make him/her aware
* to use a function in the required task-context.
*/
function requiresTaskContext($callerName)
Copy link
Member

Choose a reason for hiding this comment

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

I think better to create method in Context:

Context::required();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also thought about that option. will change it.

Copy link
Contributor Author

@staabm staabm Mar 10, 2017

Choose a reason for hiding this comment

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

now I know again why I decided against a method in Context ;-):

the message and logic here is rather task specific and Context class atm is rather generic.

    /**
     * Throws a Exception when not called within a task-context.
     *
     * This method provides a usefull error to the end-user to make him/her aware
     * to use a function in the required task-context.
     *
     * @throws Exception
     * @return Context
     */
    public static function required($callerName)
    {
        $ctx = self::get();
        if (!$ctx) {
            throw new Exception("'$callerName' can only be used within a 'task()'-function!");
        }
        return $ctx;
    }

do you still think we should move it to Context, I am fine with any solution you will accept.

Copy link
Member

Choose a reason for hiding this comment

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

I think its better.

@staabm
Copy link
Contributor Author

staabm commented Mar 10, 2017

adjusted per feedback

@antonmedv antonmedv merged commit fb745e8 into deployphp:master Mar 11, 2017
@staabm staabm deleted the req-task-ctx branch March 11, 2017 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants