Skip to content

[#30274] JFormValidator JavaScript class does not scale well#798

Closed
rmcdaniel wants to merge 1 commit intojoomla:masterfrom
rmcdaniel:30274
Closed

[#30274] JFormValidator JavaScript class does not scale well#798
rmcdaniel wants to merge 1 commit intojoomla:masterfrom
rmcdaniel:30274

Conversation

@rmcdaniel
Copy link
Copy Markdown

Joomla!'s JFormValidator JavaScript class does not scale well with large numbers
of form elements. The reason for this is that the handleResponse() method loops
through every label each time it is called. Instead of this, I have modified
the code to do this only once and store the results for future use.

Here is the relevant forum post:
http://forum.joomla.org/viewtopic.php?p=3002410#p3002410

Here is the issue tracker:
http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_id=8103&tracker_item_id=30274

@rmcdaniel
Copy link
Copy Markdown
Author

Testing went well. Any idea on if/when this will be accepted?

@rmcdaniel
Copy link
Copy Markdown
Author

We have two confirmed tests now. The patch works.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should do precomputation of label-fields in attachToForm function because if we do it in isValid every time https://github.com/rmcdaniel/joomla-cms/blob/d2c5e2c27f403bb3629ff4f4c76c53e1b1dd75b3/media/system/js/validate-uncompressed.js#L79 called we compute the same labels again and again.

I did it in https://github.com/Achal-Aggarwal/joomla-cms/compare/jquery-validation

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are going to move it anywhere, we should do as piotr-cz suggested and move it to initialize.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

both initialize and attachToForm are called once as initialize call attach to form for each form once. Even we can do the label computation in https://github.com/rmcdaniel/joomla-cms/blob/d2c5e2c27f403bb3629ff4f4c76c53e1b1dd75b3/media/system/js/validate-uncompressed.js#L72 also.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There may be more than one form on a page and therefore attachToForm may be called multiple times.

@betweenbrain
Copy link
Copy Markdown
Contributor

What is the current status of this PR? I does need to be updated with staging to be merged, but is there anything else stopping it?

@rmcdaniel
Copy link
Copy Markdown
Author

I was under the impression that pull requests before a certain date didn't have to be resubmitted to staging.

@betweenbrain
Copy link
Copy Markdown
Contributor

Yes, you are right, it does not need to be resubmitted against staging, but it is no longer in synch. It needs to be updated with the latest changes so it can be merged.

@rmcdaniel
Copy link
Copy Markdown
Author

It looks like I'll have to resubmit the pull request anyways because this needs to go in 2.5.x not master.

@betweenbrain
Copy link
Copy Markdown
Contributor

Yes, you are correct. My apologies for not noticing that. I'm going to close this issue then.

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.

4 participants