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 possibility to specify the flow of creating operation arguments #17

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

Conversation

mu4ddi3
Copy link

@mu4ddi3 mu4ddi3 commented Sep 8, 2016

No description provided.

@mikehaertl
Copy link
Owner

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.

@mu4ddi3
Copy link
Author

mu4ddi3 commented Sep 9, 2016

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

@mikehaertl
Copy link
Owner

mikehaertl commented Dec 17, 2016

@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

@mu4ddi3
Copy link
Author

mu4ddi3 commented Jan 17, 2017

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?

@mikehaertl
Copy link
Owner

@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.

@schmunk42
Copy link
Contributor

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

@mikehaertl
Copy link
Owner

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

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

Successfully merging this pull request may close these issues.

3 participants