2
\$\begingroup\$

I'm learning PHP and try to write some scripts as I go along. I wrote this script and it worked. I would like to know if this script can be improved.

The purpose of the script below is to prompt the user to input the version of the backup file, and proceed to make a backup fo the Nextcloud directory.

Thanks in advance.

#!/usr/bin/php
<?php

# prerequisites packages: apt-get install php7.4-cli php-zip

$today = date('m-d-Y');
$WWWpath = "/config/www";
$NCpath = "/config/www/nextcloud";
$user = 'xfs';
$file = 'nextcloud.zip';


chdir($NCpath);
echo "Current working directory is: " . getcwd();
echo "\n";

class Prompt
{

    protected function input()
    {
        $prompt = readline('Would you like to make a backup of the current Nextcloud ? [y|n] ' );
        echo "You selected '$prompt'\n";
        return $prompt;

    }

    public function archive()
    {
        // declare global variables
        global $WWWpath, $NCpath, $backup_version;

        $ask = $this->input();

        if(!empty($ask))
        {
            switch ($ask)
            {
                case "y":
                case "Y":
                    $backup_version = readline('Please enter a version of Nextcloud you want to backup: ');
                    echo "You input: $backup_version\n";

                    $verify = readline("Is this the correct version ? [y|n]: '{$backup_version}' ");
                    echo "You selected '$verify'.\n";

                    switch($verify)
                    {
                        case "y":
                        case "Y":
                            echo "Backup version: 'nextcloud-old_${backup_version}'\n";
                            echo "Backing up current Nextcloud directory...";
                            echo "\n";
                            $cp = ("cp -r $NCpath ${WWWpath}/'nextcloud-old_${backup_version}'");
                            $status = shell_exec("$cp");
                            $status = rtrim($status);
                            // check file copy status
                            print ((int)$status == "0") ? "File copied successfully !\n" : "Failed to copy file !\n";
                            break;

                        case "n":
                        case "N":
                            break;
                    }
                case "n":
                case "N":
                    break;
 
\$\endgroup\$
1
  • \$\begingroup\$ Please do not edit the question, especially the code, after an answer has been posted. Changing the question may cause answer invalidation. Everyone needs to be able to see what the reviewer was referring to. What to do after the question has been answered. \$\endgroup\$
    – pacmaninbw
    Commented Jul 26, 2021 at 23:39

2 Answers 2

3
\$\begingroup\$
  • using globals is a bad practice most of the time. In this case it is 100% sure it is a bad idea. It is better to pass the variables/values as parameters.
  • for a simple script like that you do not need a class or function at all. It only makes things more complicated to reason about and to understand.
  • nesting switch statements here is not needed at all.
  • the $ask variable is named really poorly. It could be $anwser or something.
  • in PHP the current standard (check out PSR) is to keep variables camelCase. snake_case is not recommended.

for the nested switch statements you can try "returning early" instead so they get un-nested e.g.

if(strtolower($answer) !== 'y') {
   exit('You cancelled the operation.');
}


$backupVersion = readline('Please enter a version ...');
// more code here

$verify = readline("Is this the correct version ... ");
if (strtolower($verify) !== 'y') {
   exit('You cancelled the operation.');
}

// the rest of the code.

\$\endgroup\$
2
  • \$\begingroup\$ How do I call the variables without using them as global? \$\endgroup\$
    – sudoi
    Commented Dec 31, 2022 at 13:32
  • \$\begingroup\$ I am not sure if you're using them anywhere else. If you use them only in the archive() function you can define them within the function and use them from there. If you use them only in that class you can define them as class properties: php.net/manual/en/language.oop5.properties.php If you need to use them in several files you can use pass them as arguments php.net/manual/en/functions.arguments.php when calling the functions... Or if you want to share them between files you can use namespaces php.net/manual/en/language.namespaces.rationale.php \$\endgroup\$
    – Hop hop
    Commented Jan 7, 2023 at 16:42
1
\$\begingroup\$

Just one quick remark: as per the docs shell_exec returns the complete output as a string. And:

This function can return null both when an error occurs or the program produces no output. It is not possible to detect execution failures using this function. exec() should be used when access to the program exit code is required.

If you want to retrieve the return code exec should be used then. And it can also return the output from execution.

The variables $WWWpath, $NCpath, $backup_version should not be defined as global in your archive function. Instead they should be function arguments.

The switch block should be outside the archive function. Get the user response first, and if the response is yes then call this function with appropriate parameters.

Misc:

  • I don't see the point of a class here, a function would be sufficient.
  • chdir is not required, just use full paths
  • you should verify that backup_version is not blank and is an appropriate value - what would happen if the user just types Enter ?
  • a switch statement is not really needed in this case, a simple if/else will do because the answer is either yes or no and you can just cast the answer to lowercase so that Y = y
  • you define a number of variables on top of your code like file, user that don't seem to be used
\$\endgroup\$
5
  • \$\begingroup\$ Hello, I'm using declare the global variables so I can use them for my 2nd part of the script. I want to use the class so the functions can be grouped together. \$\endgroup\$
    – sudoi
    Commented Jul 25, 2021 at 20:44
  • \$\begingroup\$ Thanks for the suggestions; I'd revised the script. \$\endgroup\$
    – sudoi
    Commented Jul 26, 2021 at 0:58
  • \$\begingroup\$ and empty() is inappropriate when a variable is guaranteed to be declared. \$\endgroup\$ Commented Jul 27, 2021 at 7:54
  • \$\begingroup\$ I wrote it as not empty: !empty() \$\endgroup\$
    – sudoi
    Commented Jul 28, 2021 at 23:48
  • \$\begingroup\$ What I meant was that !empty($ask) can be safely simplified to $ask when $ask is guaranteed to be declared. \$\endgroup\$ Commented Aug 23, 2022 at 5:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.