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

OC2.0.0.1b: Bug in modification system #2045

Closed
mhcwebdesign opened this issue Oct 18, 2014 · 8 comments

Comments

@mhcwebdesign
Copy link

commented Oct 18, 2014

It seems there is a bug in the now line-based OCmod, in file 'admin/controller/extension/modification.php'. For example, if I modify the original line

                            'url'  => str_replace('&', '&', $this->url->link('extension/installer/ftp', 'token=' . $this->session->data['token'], 'SSL')),

with this OCmod XML:

        .......
        <operation>
            <search><![CDATA['url'  => str_replace('&amp;', '&', $this->url->link('extension/installer/ftp', 'token=' . $this->session->data['token'], 'SSL')),]]></search>
            <add position="replace"><![CDATA['url'  => str_replace('&amp;', '&', $this->url->link('extension/installer/localcopy', 'token=' . $this->session->data['token'], 'SSL')),]]></add>
        </operation>
        ....

the replaced code always has all leading whitespaces trimmed away, like this:

'url'  => str_replace('&amp;', '&', $this->url->link('extension/installer/localcopy', 'token=' . $this->session->data['token'], 'SSL')),

instead the replaced code should keep leading whitespaces, like this:

                            'url'  => str_replace('&amp;', '&', $this->url->link('extension/installer/localcopy', 'token=' . $this->session->data['token'], 'SSL')),

We need to be able to keep proper indentations in replacement code!

@danielkerr

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2014

thx for testing

@danielkerr

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2014

this is not how vqmod works! this is a feature you want!

you should indent yourself.

@danielkerr danielkerr closed this Oct 19, 2014

@mhcwebdesign

This comment has been minimized.

Copy link
Author

commented Oct 19, 2014

Wrong, wrong, wrong!

The search should match the search expression on the line, and then just replace the match with the subsituted data. It should not do anything else, like removing leading whitespaces on the matched line. Only the matched search expression, or the subsituted data from the add-node text should be allowed to be trimmed, depending on the 'trim' attributes. See https://github.com/vqmod/vqmod/wiki/Scripting on how vqmod does it! I just tested it with QPhoria's latest VQmod 2.5.1 and my own VQmod 2.4.1 on OpenCart 2.0.0.1b.

If we want VQmod features, with just using a different OCmod XML syntax, then your current code is wrong. If you like, I can easily implement your planned OCmod XML syntax, I already have VQmod-XML anyway, the main change would only be the relocation of the 'position' and 'offset' attributes from the 'search' node to the 'add' node, and some other minor changes ('name' instead of 'id', or introducing the 'code' and 'link' nodes that didn't exist in VQmod).

@jamesallsup

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2014

@mhcwebdesign I think what might be useful if you/us/users of vqmod did a detailed explanation of what each action does and how it acts (currently) to aid Daniel get it perfect. Personally I did think that searching on a line with replace etc, the new injected/ replaced code wouldneed to be indented to match (I.e. it would start at column 1 if you didn't space or tab).

Pm me of the forum if you want me to set up a shared google doc for us all!

J

@mhcwebdesign

This comment has been minimized.

Copy link
Author

commented Oct 19, 2014

James, the replace operation from above example works like this in VQmod:

  1. optionally trim the search expression as per the search-node's 'trim' attribute
  2. find the line with the matched expression
  3. trim the replacement text as per the add-node's 'trim' attribute
  4. replace the matched string with the replacement text

The matched string may or may not be a sub string on the matched line, it could be anywhere on the line. The line containing the match should not be altered beyond the actual replacement of the matched text. No additional trimming of leading whitespaces on the line containing the match text! It's really a simple logic, nothing magic!

@jamesallsup

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2014

Yeah I follow, but it's not for me (who has used vqmod plenty)...I/we need to give Daniel as many examples, explanations, help etc as possible to ensure its perfect. Pretty sure you will appreciate Daniel may not have used vqmod as much as us! So its just about getting the right info across to him to make sure the next release is as good as it can be :)

I'm not heading up the dev side for the ocmod system just trying to advise where possible but I know there is plenty of devs inc yourself who use it probably far more extensive with more finite knowledge of the minor workings.

I'll skype with Daniel again tomorrow and also try to do some test scripts for him to play with. If you have any examples too be sure to pass them over as well...if you want to chat on skype let me know and i'll pm you my address.

J

@mhcwebdesign

This comment has been minimized.

Copy link
Author

commented Oct 19, 2014

I don't use Skype, but I am happy to create a pull-request sometime tomorrow for the 'admin/controller/extension/modification.php' with a complete OCmod-XML system that behaves like VQmod and has all its features. It's just a matter of changing the XML-syntax from VQmod to OCmod, keeping the VQmod features.

@JAY6390

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2014

I've not gone through the code, but from the input and output, it looks like the line is being replaced completely. That's not how vQmod works. It replaces using str_replace, with <search> as the needle, <replace> as the replacement text and the whole line as the haystack. The line itself does not get trimmed, and the indentation on that line should remain intact. There are examples for most of the ways vQmod works using various parameters and to be honest it's not rocket science what it does. These are basic search and replaces. The trim parameters should not affect the line content, nor should the line content be trimmed in any way. @mhcwebdesign has got it spot on in steps 1 - 4 above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.