php-shellcommand icon indicating copy to clipboard operation
php-shellcommand copied to clipboard

Add possibility to specify the flow of creating operation arguments

Open mu4ddi3 opened this issue 8 years ago • 7 comments

mu4ddi3 avatar Sep 08 '16 13:09 mu4ddi3

Hmm, I'm not against adding more flexibility here, but can you show me some examples, which arguments you want to render with this? If we change this, we really need to be sure to find a generic solution that works for all situations.

mikehaertl avatar Sep 08 '16 14:09 mikehaertl

Hi mike, sure i can. Without change there is a global flag for all operation arguments who decides whether they should be escaped with "" or not. Its OK, but i added a new operation to php-pdftk (attachFiles()) to handle adding attachments into PDF file, and this new operation has a specific syntax. The syntax of this operation is : "pdftk input.pdf attach_files path_to_file_1 [path_to_file_2] [to_page 5]". path_to_file_1 and and other paths they need to be escaped because there could be space inside etc, but [to_page X] could not be escaped because it will be treated as another path to attachment. So within one operation we have for example three arguments of which only two need to be escaped. Third cant. Because of this flow, one global flag for operation cannot be used to control escaping of all its arguments.

I decide to extend mechanism of defining arguments, to bring (if necessary) ability to specify private escape flag (which will have priority over global) for each argument individually. Here in php-schellcommand i just added ability to process that kind of defined argument, but in php-pdftk you can see a method addOperationArgument(), which is the best example of what i am talking about.

And a little bit of code example which should be as good explanation as verbal :

Use example: (as i said changes here are consequences of extending php-pdftk, so i give a use example of it)

1.Adding attachment (new operation)

$pdf = new Pdf;
$pdf->addFile('file1.pdf', 'A')
 ->attachFiles(['attachment.pdf','image.jpg']);
 ->saveAs('out.pdf');

attachFiles method looks like this: -it define operation -it sets arguments old way -and add last argument new way

public function attachFiles($files, $toPage = null)
    {
        $this->getCommand()
            ->setOperation('attach_files')
            ->setOperationArgument($files, true) //set arguments which should be escaped
            ->addToPage($toPage); //add argument which should't be escaped
        return $this;
    }

and addToPage() method which is used to add argument new way

 public function addToPage($toPage = null)
    {
        $this->checkExecutionStatus();
        if ($toPage!==null) {
            $this->addOperationArgument('to_page', $toPage, ' ', false);
        }
        return $this;
    }

doing all the dirty stuff:

    public function addOperationArgument($argument, $value = null, $separator = null, $escape = null)
    {
        $this->_operationArgument[] = $value===null ? $argument : array($argument, $value, $separator, $escape);
        return $this;
    }

Ps. I just noticed i did not make a pull request for php-pdftk before so you cannot see what i'm talking about. I did it now

mu4ddi3 avatar Sep 09 '16 07:09 mu4ddi3

@mu4ddi3 Hmm, I'd prefer to separate things a bit more. How about this:

function addArgs($key, $values, $separator = ' ', $escape = null)

$values = [
    'value1',
    'value2',
    ['value3', true], // if array: [$value, $escape]
];
  • Additional method addArgs() (note the extra "s") for args with more than 1 value
  • $escape overrides the $escapeArgs setting
  • Each value in $values can either be a string or an array like [$value, $esc], where $esc overrdies $escape and $escapeArgs

mikehaertl avatar Dec 17 '16 12:12 mikehaertl

Hi @mikehaertl, sorry for delay. Change of the year is always a stressful time in my job - deadlines etc. I checked your proposition and it is more simplified, and the separation is also a good thing. "addArgs" function is clear and easy to understand.

What now should i make a changes or you do it on your project?

mu4ddi3 avatar Jan 17 '17 10:01 mu4ddi3

@mu4ddi3 Thanks for the feedback. Same here: Busy times. It will take a while (some weeks), but I will take care of the changes and let you know. Maybe you can then help testing.

mikehaertl avatar Jan 17 '17 10:01 mikehaertl

@mikehaertl Speaking about testing, you might wanna enable some newer PHP versions in Travis also ;)

schmunk42 avatar Jan 17 '17 11:01 schmunk42

@schmunk42 Feel free to file a PR for this ;).

mikehaertl avatar Jan 17 '17 12:01 mikehaertl