User tests: Successful: Unsuccessful:
JForm::bind method was relying on getProperties() which returned only the attributes of the given object. However in case one of these attributes happens to be a JObject instance itself, then JForm::bindLevel would fail there.
In particular model's getItem function tends to return a nested JObject always.
JArrayHelper call would take care of JObject to Array conversion recursively.
Thanks for the tip. ![]()
However I'm looking for some garbage cleanup. Those commits are still there but hidden.
| Labels |
Removed:
?
|
||
| Title |
|
||||||
| Status | New | ⇒ | Pending |
| Title |
|
||||||
| Category | ⇒ | Libraries |
Hello @izharaazmi
Thank you for your contribution.
The last comment here was on 8th June 2014. So the question is, is this issue/pull request still valid?
if so please provide clear test instructions to be able to test / reproduce this issue.
If no reply is received within 4 weeks we will close this issue.
Thanks for understanding!
| Status | Pending | ⇒ | Information Required |
I dropped this idea as the JObject got deprecated. However the issue is
still valid as long the JObject is being used in Model's getItem() method.
Should I update the PR, or just close it?
On Aug 21, 2015 12:38 AM, "RolandD" notifications@github.com wrote:
Hello @izharaazmi https://github.com/izharaazmi
Thank you for your contribution.
The last comment here was on 8th June 2014. So the question is, is this
issue/pull request still valid?
if so please provide clear test instructions to be able to test /
reproduce this issue.
If no reply is received within 4 weeks we will close this issue.Thanks for understanding!
This comment was created with the J!Tracker Application
https://github.com/joomla/jissues at issues.joomla.org/joomla-cms/3737
http://issues.joomla.org/tracker/joomla-cms/3737.—
Reply to this email directly or view it on GitHub
#3737 (comment).
Yes, it should be updated. Even though something is deprecated, it should continue to function correctly until it is removed.
@izharaazmi If you can provide the test instructions soon that would be appreciated, I have a group of people testing issues tomorrow and I would be able to include this one. Thanks.
To reproduce this we'd need the recursive JObject returned by the JModelAdmin being passed to JForm::bind() directly.
For quick test do the following:
Create a form XML with a field with xpath something like /fields[@name="level1"]/fields [@name="level2"]/field[@name="field1"]
Motive is to make the field deep nested.
Pass the JForm::bind a recursive JObject created from following object:
{level1: {level2: {field1: field_value}}}
Binding level1 would be fine, but level2 will cause error as the protected property $_error of JObject is included in the converted array in the bindLevel method of JForm which is preceded by a null byte.
This holds true for all subsequent levels.
My PR fixes this JObject to array conversion by using ArrayHelper instead of using getProperties method.
Sample code for test:
$form = JForm::getInstance('test', '<form><fields name="level1"><fields name="level2"><field name="field1" type="text"/></fields></fields></form>');
$array = array('level1' => array('level2' => array('field1' => 'field_value')));
$data = JArrayHelper::toObject($array, 'JObject');
$form->bind($data);
echo '<pre>';
print_r($data);
print_r($form);
echo '</pre>';
Tested it but I couldn't see any differences between the changes.
So I couldn't reproduce it.
Make sure you have set error_reporting = ON
I just verified and it shows the following warnings without this patch. After this patch the warnings are gone.
Warning: SimpleXMLElement::xpath(): Unfinished literal in /Users/izharaazmi/Sites/joomla/libraries/joomla/form/form.php on line 1526
Warning: SimpleXMLElement::xpath(): xmlXPathEval: evaluation failed in /Users/izharaazmi/Sites/joomla/libraries/joomla/form/form.php on line 1526
Warning: SimpleXMLElement::xpath(): Unfinished literal in /Users/izharaazmi/Sites/joomla/libraries/joomla/form/form.php on line 1526
Warning: SimpleXMLElement::xpath(): xmlXPathEval: evaluation failed in /Users/izharaazmi/Sites/joomla/libraries/joomla/form/form.php on line 1526
The warning occurs at following line:
if ($tmp = $element->xpath('descendant::field[@name="' . $name . '"]'))
Tested with Joomla version: 3.4.4-dev development
1) Created a little component which contained the example code of @izharaazmi
2) Patch was not applied yet: I got the same warnings as @izharaazmi
3) Patch was applied: The warnings were no longer shown and nothing was written into the php_errors.log anymore
Test was successful!
| Status | Information Required | ⇒ | Ready to Commit |
Just tested this successful with the sample code by @izharaazmi. Based on the test by @kathastaden I'm RTC'ing here. Thanks!
| Labels |
Added:
?
|
||
| Milestone |
Added: |
||
| Milestone |
Added: |
||
| Milestone |
Removed: |
||
| Status | Ready to Commit | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2015-09-08 23:47:16 |
| Closed_By | ⇒ | roland-d |
| Labels |
Removed:
?
|
||
| Milestone | |||
| Milestone |
Added: |
||
| Milestone |
Added: |
||
| Milestone | |||
Hi,
Thank you for all of the work on Joomla 3.5 so far!
I've just tested our code on Joomla 3.5 Beta and something didn't work well, then I did a back tracking and found the root cause is the change in this pull request.
In our case, our Model return $item which has the following structure:
$item (JObject)
|-- $foo (Array)
|-- |-- 0 => (stdClass)
|-- |-- 1 => (stdClass)
|-- |-- 2 => (stdClass)
This structure has been worked fine since Joomla 3.0.0 and stopped this morning because JForm will automatically convert all nested objects to array silently.
We can make a work around this but I am afraid of how many extensions out there are relying on the old JForm behavior.
Hi @izharaazmi, can you please do a PR against staging with your commit (izharaazmi@64bc802)? Thanks!
@Kubik-Rubik It's on the way!
You can use git squash to make a single commit from these 6 commits. More about it is at http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html
For current case
git rebase -i HEAD~6and then put "pick" in front of commit ccb2d74 and "squash" in front of others.