String Manipulation to SimpleXML and DOMDocument

stutteringp0et commented

2017-12-03 08:10:38 +00:00

(Migrated from github.com)

I removed the string manipulation in favor of something more developer friendly. The drawback is that the resulting XML is not as pretty, although I did run the processed fields through DOMDocument pretty printer. The entire resultant XML could be run through a pretty printer, but still won't achieve the tab indented node attributes.

The entire process could be made much more efficient, but this was just an initial conversion.

Summary of Changes

Removed string manipulation in favor of direct xml manipulation

Testing Instructions

generate a form

Expected result

a form is generated

Actual result

a form is generated

Documentation Changes Required

I removed the string manipulation in favor of something more developer friendly. The drawback is that the resulting XML is not as pretty, although I did run the processed fields through DOMDocument pretty printer. The entire resultant XML could be run through a pretty printer, but still won't achieve the tab indented node attributes. The entire process could be made much more efficient, but this was just an initial conversion. ### Summary of Changes Removed string manipulation in favor of direct xml manipulation ### Testing Instructions generate a form ### Expected result a form is generated ### Actual result a form is generated ### Documentation Changes Required

👎 1

Llewellyn commented

2017-12-03 09:24:22 +00:00

I tried review your changes but since your spacing is a total mess I can't do any review for almost the whole file changed. First fix the spacing....

Llewellyn commented

2017-12-03 09:24:48 +00:00

So that only the area you worked on shows as changed in git.

Llewellyn commented

2017-12-03 09:28:48 +00:00

This is ridiculous

This is ridiculous ![image](https://user-images.githubusercontent.com/5607939/33524005-04d7d9b6-d81d-11e7-8923-661fb64d0d2f.png)

Llewellyn commented

2017-12-03 09:35:05 +00:00

If you can not with the change produce the same neat as you call it pretty print result, I can tell you now this is not getting merged. Sorry. I need the xml to look hand written, and therefore perfectly neat and in order with comments and clean.

I spend tone of hours to achieve that, so if you can improve, then do. I for one can not see why string manipulation is any less developer friendly? if what it produce is more superior and clean result.

If you can not with the change produce the same neat as you call it *pretty print* result, I can tell you now this is not getting merged. Sorry. I need the xml to look hand written, and therefore perfectly neat and in order with comments and clean. I spend tone of hours to achieve that, so if you can improve, then do. I for one can not see why string manipulation is any less developer friendly? if what it produce is more superior and clean result.

👍 1

stutteringp0et commented

2017-12-03 16:33:23 +00:00

(Migrated from github.com)

I put a lot of work into it, so I'm not giving up. I can tell you put a lot of work into the string manipulation, but the idea of having to run multiple tests to determine if you should close a tag seems like a waste of time when the PHP xml classes do that automatically.

So. I think I should first address the issue you have with my code formatting. I tab my way to my desired indention level and when I'm done I use the Source > Format utility of NetBeans. I can adjust the settings, but I need to know what the standard is. It looks like you use an 8 character tab indent. Is that correct? I'll adjust my IDE settings to put function opening curly braces on the 2nd line.... Do you have your code style prescribed somewhere?

As far as reaching the "hand written" output look you want - I think I can do an XSLT transform on the finalized XML to achieve that. I've never used XSLT for that, but what the heck.

I put a lot of work into it, so I'm not giving up. I can tell you put a lot of work into the string manipulation, but the idea of having to run multiple tests to determine if you should close a tag seems like a waste of time when the PHP xml classes do that automatically. So. I think I should first address the issue you have with my code formatting. I tab my way to my desired indention level and when I'm done I use the Source > Format utility of NetBeans. I can adjust the settings, but I need to know what the standard is. It looks like you use an 8 character tab indent. Is that correct? I'll adjust my IDE settings to put function opening curly braces on the 2nd line.... Do you have your code style prescribed somewhere? As far as reaching the "hand written" output look you want - I think I can do an XSLT transform on the finalized XML to achieve that. I've never used XSLT for that, but what the heck.

stutteringp0et commented

2017-12-03 16:34:43 +00:00

(Migrated from github.com)

About the XML comments - I did take special care to preserve them in their original configurations and locations (indention fixes withstanding)

Llewellyn commented

2017-12-03 17:00:57 +00:00

My best advice to you is copy the area you worked on out of the file, then hard reset and fix the indentation of what you are doing to yes 8 per tab and only add the changes back without formatting the whole file.

I really do not want to go over work that is just spacing issues. I will want to just look at your work, not indentation stuff.

I am open to do things better, as long as it does not take us back in aesthetics nor in functionality required for behaviors and checks.

I feel like this change is going to cause me more work... but okay let me see.

My best advice to you is copy the area you worked on out of the file, then hard reset and fix the indentation of what you are doing to yes **8 per tab** and only add the changes back without formatting the whole file. I really do not want to go over work that is just spacing issues. I will want to just look at your work, not indentation stuff. I am open to do things better, as long as it does not take us back in aesthetics nor in functionality required for behaviors and checks. I feel like this change is going to cause me more work... but okay let me see.

Llewellyn commented

2017-12-03 17:03:12 +00:00

I am trying to keep to the Joomla Code style as described in the contributer guide.

I am trying to keep to the Joomla Code style as described in the [contributer guide](https://github.com/vdm-io/Joomla-Component-Builder/blob/master/.github/CONTRIBUTING.md).

Llewellyn commented

2017-12-03 17:05:03 +00:00

Yet I know there are areas in the compiler that has not followed that, so it needs refactoring. But for now that must wait... there is to much other things for me to do.

Llewellyn commented

2017-12-03 17:09:08 +00:00

you did not do what I said so the file stil has way to many changes sorry...

stutteringp0et commented

2017-12-03 18:32:03 +00:00

(Migrated from github.com)

I'm holding off on any more changes to this PR until I hear from you.

I think the indention of the inserted XML block (matching the existing indent level) would be best fixed by running through tidy before writing the xml output file. If you accept this PR, I'll modify the file write operation to tidy before write.

I'm holding off on any more changes to this PR until I hear from you. I think the indention of the inserted XML block (matching the existing indent level) would be best fixed by running through tidy before writing the xml output file. If you accept this PR, I'll modify the file write operation to tidy before write.

Llewellyn commented

2017-12-03 18:37:20 +00:00

Please add commenting to the new functions... let do that in the files where most functions have their commenting in place.

I also think you will have to fix the indentation before.

Please add commenting to the new functions... let do that in the files where most functions have their commenting in place. I also think you will have to fix the indentation before.

stutteringp0et commented

2017-12-03 19:13:07 +00:00

(Migrated from github.com)

OK, I fixed indent and commented the functions

ro-ot commented

2017-12-03 19:16:29 +00:00

(Migrated from github.com)

Hey commenting is always expected, only @Llewellynvdm gets away, and not for long 👍

Hey commenting is always expected, only @Llewellynvdm gets away, and not for long :+1:

stutteringp0et commented

2017-12-03 19:21:27 +00:00

(Migrated from github.com)

I wasn't going to spend time adding comments until there was a light at the end of the tunnel and I was certain it wasn't a train. The initial reception was a bit cold. I'm glad the formatting fixes warmed things up a bit. This will open the door for a lot of future improvements.

ro-ot commented

2017-12-03 19:24:55 +00:00

(Migrated from github.com)

Hey noting cold here just been us two for long time, hope you understand the love 💯

Hey noting cold here just been us two for long time, hope you understand the love :100:

ro-ot commented

2017-12-03 19:29:08 +00:00

(Migrated from github.com)

Always room for more, but you have to admit you had a made a mess of the formatting lol. Did you get my comments surrounding those two areas?

stutteringp0et commented

2017-12-03 19:31:54 +00:00

(Migrated from github.com)

I didn't see that. What's the formatting standard? I think I had my IDE set to the Joomla standard...but I did re-install Linux last month (upgraded storage to a mirrored SSD hardware RAID array - and it's stupid fast!)

ro-ot commented

2017-12-03 20:09:15 +00:00

(Migrated from github.com)

Just ran a test on a huge project of mine, did blank install, installed this version and then as expected:

String could not be parsed as XML

So we have a problem with those PHP strings in the XML.

Just ran a test on a huge project of mine, did blank install, installed this version and then as expected: ``` String could not be parsed as XML ``` So we have a problem with those PHP strings in the XML.

stutteringp0et commented

2017-12-03 20:36:37 +00:00

(Migrated from github.com)

Could you post the offending xml?

ro-ot commented

2017-12-03 20:39:26 +00:00

(Migrated from github.com)

That is my point there is thousands of fields in my component and which one is it now? Do you have the time to search a thousand fields, I do not.

Llewellyn commented

2017-12-03 20:50:02 +00:00

Please check this info to give us the maintainers permission to make changes to this code. Since I have tried to help you here in making some changes but don't seem to succeed in pushing it up. Keeps on creating a new branch... maybe I am doing something wrong. Do you know how I can contribute to his pull request?

Please check this [info](https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/#enabling-repository-maintainer-permissions-on-existing-pull-requests) to give us the maintainers permission to make changes to this code. Since I have tried to help you here in making some changes but don't seem to succeed in pushing it up. Keeps on creating a new branch... maybe I am doing something wrong. Do you know how I can contribute to his pull request?

ro-ot commented

2017-12-03 21:25:57 +00:00

(Migrated from github.com)

found one:

<field 
	type="text"
	name="worldzone" // selfde as die databasis
	label="Worldzone"
	size="40"
	maxlength="50" 
	default=""
	description="The zone this country belongs to"
	class="text_area"
	filter="INT"
	required="false"
	message="Error! Please add zone code here."
	hint="only number"
/>
// hey moet asblief nie die een verander nie!!!

I left some notes in here for other team members, this is a great example of why I think @Llewellynvdm is right to request you take just two small steps back.

You did a great job on the other area why not work with us on this. I am telling you the call here to remain resilience is the right one.

found one: ``` <field type="text" name="worldzone" // selfde as die databasis label="Worldzone" size="40" maxlength="50" default="" description="The zone this country belongs to" class="text_area" filter="INT" required="false" message="Error! Please add zone code here." hint="only number" /> // hey moet asblief nie die een verander nie!!! ``` I left some notes in here for other team members, this is a great example of why I think @Llewellynvdm is right to request you take just two small steps back. You did a great job on the other area why not work with us on this. I am telling you the call here to remain resilience is the right one.

stutteringp0et commented

2017-12-03 21:27:25 +00:00

(Migrated from github.com)

I'll change it back to string search, but I have to question the purpose of storing it as xml if you can't parse it as xml.

Why xml, why not ini, or csv key value pairs or just random crap with key value pairs injected randomly?

As far as 1000s of fields, yes - I would find the malformed xml immediately. It might take 10 minutes to write the code to find it.

I'll change it back to string search, but I have to question the purpose of storing it as xml if you can't parse it as xml. Why xml, why not ini, or csv key value pairs or just random crap with key value pairs injected randomly? As far as 1000s of fields, yes - I would find the malformed xml immediately. It might take 10 minutes to write the code to find it.

ro-ot commented

2017-12-03 21:49:31 +00:00

(Migrated from github.com)

Really? can we just avoid taking this personal, sure we could add checks to detect which fields are not valid xml.

I think Llewellyn chose the xml layout since it looks familiar to how fields are done in Joomla, I read the code and therefore know those values are being grabbed one at a time. So I started taking advantage of the freedoms. Don't let this become a big deal to you.

This is really not so big a deal, it is just in all my test I ran with your code my fields changed there names (so I had install issues with database mismatch) that I don't think this is the time and place to take on the world.

Not wile we are in the middle of a huge upgrade, don't know if you are aware of the coming changes.

I will keep this improvement in the back of my mind, and who knows when the time is right we can take this up again.

Please just not now.

Really? can we just avoid taking this personal, sure we could add checks to detect which fields are not valid xml. I think Llewellyn chose the xml layout since it looks familiar to how fields are done in Joomla, I read the code and therefore know those values are being grabbed one at a time. So I started taking advantage of the freedoms. Don't let this become a big deal to you. This is really not so big a deal, it is just in all my test I ran with **your code** my fields changed there names (so I had install issues with database mismatch) that I don't think this is the time and place to take on the world. Not wile we are in the middle of a huge upgrade, don't know if you are aware of the coming changes. I will keep this improvement in the back of my mind, and who knows when the time is right we can take this up again. Please just not now.

stutteringp0et commented

2017-12-03 22:15:39 +00:00

(Migrated from github.com)

I'm not trying to make it personal - I often use sarcasm

You can still have notes - just make them compliant with xml:

<field 
type="text"
name="worldzone" name:note="selfde as die databasis"
label="Worldzone"
size="40"
maxlength="50" 
default=""
description="The zone this country belongs to"
class="text_area"
filter="INT"
required="false"
message="Error! Please add zone code here."
hint="only number"
 />
<!-- hey moet asblief nie die een verander nie!!! -->

If the system stores XML - I don't want to parse text, I want to query the XML

Updated the PR to use string search to allow for malformed XML

I'm not trying to make it personal - I often use sarcasm You can still have notes - just make them compliant with xml: <field type="text" name="worldzone" name:note="selfde as die databasis" label="Worldzone" size="40" maxlength="50" default="" description="The zone this country belongs to" class="text_area" filter="INT" required="false" message="Error! Please add zone code here." hint="only number" />  If the system stores XML - I don't want to parse text, I want to query the XML Updated the PR to use string search to allow for malformed XML

stutteringp0et commented

2017-12-03 22:17:01 +00:00

(Migrated from github.com)

@Llewellynvdm - Requested changes have been made

stutteringp0et commented

2017-12-03 23:02:51 +00:00

(Migrated from github.com)