Removing repetable fields from site view, custom admin view, and admin view. #138
Labels
No Milestone
No project
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: joomla/Component-Builder#138
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "staging"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
I tested. Simply installing into a new site, everything appears to be fine. However, I installed on a clone of my dev site, attempted to open a large admin view that I have, and got an out of memory errors, and I have my PHP memory set to 256MB.
Fatal error: Allowed memory size of 268435456 bytes exhausted (tried to allocate 63889187 bytes) in /var/www/dev/plugins/system/debug/debug.php on line 1225
I increased memory_limit to 512M and it loaded, but took forever to do so. I'm guessing that the high memory usage is due to the number of input vars that it's having to load/process.
I am wondering if we should not port the repeatable field into the JCB core... I mean if Joomla stops support for it... we can still use it without them. That field was a powerhouse...
Lets do a test @mwweb if you delete line 82 in component->views->admin_view->tmpl->edit.php is there a dramatic improvement in the page load?
I have do so and the page speed is back to normal... so I think we move those two concepts out onto its own view/table. Please confirm your side.
After seeing the tremendous slow page load, I agree we must move the fields out. But still display the fields with some details via ajax call in the fields tab, with buttons to edit.
@Llewellynvdm let me know what I can help with.
Hey we had to drop the repeatable field, it was just a buggy nightmare in the Javascript. The direction you are going will work, and make JCB even better believe me.
I removed that line, and the view does load quickly once again.
On my big view, it loaded, but only with the Settings tab. all the others were missing. But, it looks like this may be due to a jQuery error, likely due to the size of the view (number of fields, tabs, etc.)
Uncaught RangeError: Maximum call stack size exceeded
I think I'm off to sleep. It's almost 3am here.
@ro-ot I need help with the ajax... I will setup the new fields view. Not sure what to call it yet. But okay... I will start on that right away. Soon as I am ready I will push the changes.
Maybe just call it
admin_fields
andadmin_field
with no direct menu link, only via the admin views. Once you have all in place I will dump the ajax in. My M-JCB is out of sync with yours, so maybe I just give you the code and you add it.Stunning!
Long story..
Okay @mwweb & @ro-ot I have decoupled the fields and conditions. Please test and give feedback.
Note this is not production ready!
@ro-ot You will see there is a placeholder for the fields to load via ajax... if you can just build the PHP function that returns the ready build HTML to the ajax call then I will do the rest.
So the table from which to get the fields is admin_fields and the table to get the conditions is admin_fields_conditions
OK. "Smaller" view load fine, and it looks like the new fields and conditions load fine.
However, on a large view with lots of fields and conditions, I'm still getting a jQuery error:
At some point, we may want to look at reorganizing the layout of the settings tab. Notice how it shows off page on the right:
Some times you need to just do a ctrl-f5 page refresh to get the newest javaScript and stuff. I don't see those errors.
Any suggestions on the page layouts?
Perhaps just a single column.
On the jQuery error, I am still receiving it even after a ctrl-F5, and even clearing all my browsing data. This was in Chrome. I then tried in Internet Explorer, and in the end it crashed IE, after continually saying the a scripts was taking too long to respond.
I am making a backup of my entire dev site, and I could put it up on my web server if you would like the credentials that I could send to you, and you could see the error in action.
I have to say that in the end, I like how you did the buttons to take to the admin_fields and admin_fields_conditions views. I might look at "breaking apart" my big view, and splitting it to separate tables, with buttons doing similar to what you have done. I'll have to think on that, since it would be more learning on how to do that.
It actually looks like it may be related to the number of fields. From the DB:
In doing some digging:
In MySQL, the text field type allows a maximum 65535 characters. If it's over that, then the error above. The suggestions were to use largetext, or even better medium or large blob.
I'm not sure if that will resolve the issue or not though. I manually changed addfields to largetext, and the error isn't there, but still getting the jQuery error.
Are you saying a text column (in the DB) is to small? Wow how many field do you have?
O are you taking about a view in your component? Well if that is the case... yes you need to decoupled 👍
I'll do more tests later. I emptied the fields in the DB, and "error" still persists, even after a ctrl f5.
In what view are you seeing this? in the admin view? or in the admin fields view?
I still have to clean out some scripts from the admin_view
One question, and should probably be somewhere else...
In this build, the new hidden views: admin_fields and admin_fields_conditions are showing as a single view in admin/components/com_componentbuilder/views. By default, JCB requires Name (Single Record) and Name (List of Records). In this version you only have a Single Record (no list of records). Was that accomplished via custom code, or some other way?
It is a default JCB behavior if a number of things lineup. Then I also made the field readonly, since if all the things lineup the correct view is always selected. So the user does not need to select it.
What must lineup?
Well the sort answer is, the field name is "admin_view" and the referral view is "admin_view" and it always pass a referral ID in the url. So the first time you open the admin_fields view it uses the
ref=admin_view
andrefid=1
to automatically select the correct view from the list. JCB will always do that, but only on the first load when it has no values set, and has not been saved. Since I know each admin_view should only have one admin_fields linked to it... I allow this automation and then with the readonly switch in the field prevent the user from changing it. Hey it can be bypassed, but then the user will know that it broke because of him/her bypassing this automation.So it is still a custom list view, that gets auto selected and is set to readonly.
There is so many things like that... that i have forgotten of some of them already and need to open old component to see how I did it... then I realize hey JCB already has that trick in place... lol so I end up learning JCB...
This last push has the new layout 👍
To answer the question: In what view are you seeing this? in the admin view? or in the admin fields view?, it is in admin view that i am seeing the jquery error, and the message about the field length was seen in admin_view, admiin_view_fields, and admin_views_conditions
I think we should work on a limiting factor around how many fields should be allowed per/view at this time it is set to 800, but that is way to many... I think around 200 is a more reasonable number.
I mean we must realize that 200 fields can mean 200 columns in the table and that is bad practice.
I have pushed Joomla to its limits with JCB and it worked with repeatable fields, but this subform is not allowing this huge amounts of values to work well.
We have now completely isolated the fields to its own table/view and still it can't handle so many fields.
I don't know what more can be done, without resorting to custom scripting (outside of Joomla) You see I have not added any script to the page besides the Joomla default scripts... and if they give errors we are probably going to cut back on the number of fields allowed per view.
Test this last push... I am off to bed it is 6am here. Was a long hard coding night for me. The storm is almost over... @ro-ot please bump that script for me.
The new layout looks good.
However, I think there is something more that is causing that jQuery Range Error. I just tested with one of my smaller views (25 fields, including the default fields the JCB adds such as id, created, published, etc), and I got the range error. Out of 14 views, most with around 25 or less fields, only 4 opened without any errors.
I can't see that I have done anything "abnormal", except for adding fields and tabs, which is normal.
@mwweb I also do not seem to get these errors. Are you 💯% sure that your browser cache has been cleared. Also looking at your error message, it seems you JavaScript is being cached, so also clear your server cache, or just turn it off.
@Llewellynvdm I think you did a great Job!
Regarding the allowed fields size. I have these thoughts.
Loading the fields back via Ajax
I will dig into it more today, but the error occurred on different sites i set up on my localhost. And occurred on a copy of the site i uploaded to my web server. But like i said, I'll dig into it more today.
I can't say definitively, but it may have to do with something in the data conversion from repeatable fields to subforms. I just went and recreated two of my views that were throwing that jquery range error. I recreated them with identical tabs, permissions, fields, conditions, everything.
The result? No errors.
Can you give us the two json objects? The one done by the JCB conversion and the one done when you manually saved the subform?
Definitely differences in the 2 JSON's.
Original
New
OK. I think it's in conjunction with the differences in the addfields and addconditions when converted, and also to the below.
I tested this by copying the content from the "recreated" fields and conditions to the old, and still had the error. I then saw that the only major difference was with addtables in admin_view. The old had contents, the new didn't. With the different JSON in place, and deleting the contents of addtables, it started working.
In the admin_view table, there is a field called "addtables". This field is populated in the one throwing an error, but is not populated in the one that isn't. As a test, I went into the one that is throwing the error and emptied "addtables".
No errors any more.
The only difference I see is that some values are missing in the second. This could be because @Llewellynvdm changed the field type from radio to checkbox for some of these fields, and it only has one value 1 not zero. This could be the issue.
If that is the case, I would say ignore this since the first time you save the admin fields view it will correct this issue.
It's more than just the new tables admin_view_fields and admin_view_conditions, as in my testing that alone didn't resolve the error. Once I did a compare of the 2 records in admin_view, I saw that addtables was populated in the original record, but not the new. I deleted from the original record, and it started working.
So, something between the new tables and the addtables looks like it may be the culprit. Also, not sure that it's needed any longer, but in admin_view the addfields and addconditions fields are still there.
I have a JCB Package of the component, from JCB 2.5.5, that I can provide if that would make it easier to debug.
From my own testing I also found that not all conversions were done, so I added a few fixes around JCB to deal with this.
I think we can move forward with this as it clearly is simply huge datasets that are to big to convert all at once. So adding little conversion scripts in the model everywhere where these fields gets loaded is what I have done. So that even after the install if a convention was not done completely it will be done the moment you open the view.
I'll test new version when available.
About the fields still being in the database, yes we will only finally remove them few update further, just until we are sure all is working as planned. So I am not clearing them out just yet.
I have tested it and it works great.
The import and export still broken.
But the admin view works very well now.
So the new error handler is in place. I see you also updated all the files 👍
Only 50? is that not to low?
Well it is only a notice and in reality they can add up to 800. I just found the moment you go beyond that number the new field views really slowdown. So telling them that they should try to stay below 50 is a good idea.
Do you think we need to explain more in terms of why it is bad practice?
@mwweb can you give me any feedback on those errors if they are now resolved with this last few updates. It will be helpful, thanks!
My apologies, I have been traveling. I just did a reinstall back to 2.5.5, then installed the most recent build. From what I can see, by opening each view, everything is loading correctly.
I should have the final release out today! I am almost done with the import of JCB packages to ensure backward compatibility. So when I release it, I would need testing to check backward compatibility. I mean to insure the packages that were exported in the past will import and end in the right place.
I am still having issue on the import of New JCB packages. @ro-ot can you take a look at this and see what I missed?
@Llewellynvdm the JCB package export seems to be the issue. Not all the items are being exported, it seems like some items are missed.
That is all I have for now.
Okay now we have the new export and import working. Still need to do some checks on old packages. Would appreciate some feed back on this last push.
Looks good, I would give it a 👍