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

Fix incorrect row count on nested subforms #22757

Merged
merged 2 commits into from
Nov 5, 2018
Merged

Conversation

continga
Copy link

Pull request for Issue #22441 (and also #22690 ) - see those issues for testing instructions.

Summary of changes

We were incorrectly using the lastRowNum property inside of the subformRepeatable object due to
legacy reasons (the logic using a lastRowNum property has been overtaken from history, see commit
f2a9874 ).

What happened is that we always were incrementing the lastRowNum property by 1 inside of the
fixUniqueAttributes method, whilest we call that method multiple times recursively, when we have
nested subforms. So having a 2-level subform meant we would increment the lastRowNum twice for every
added row, effectively meaning we would skip certain indices and hence totally lose the connection
between the actual count of existing rows and their index. Actually we do not need the lastRowNum
property, as the current logic works well without it just by calculating the count of rows directly
in the addRow method. So removing the lastRowNum property is the best solution.

@gaelicwinter and @dawe78 can you please this PR? Thanks!

We were incorrectly using the lastRowNum property inside of the subformRepeatable object due to
legacy reasons (the logic using a lastRowNum property has been overtaken from history, see commit
f2a9874 ).

What happened is that we always were incrementing the lastRowNum property by 1 inside of the
fixUniqueAttributes method, whilest we call that method multiple times recursively, when we have
nested subforms. So having a 2-level subform meant we would increment the lastRowNum twice for every
added row, effectively meaning we would skip certain indices and hence totally lose the connection
between the actual count of existing rows and their index. Actually we do not need the lastRowNum
property, as the current logic works well without it just by calculating the count of rows directly
in the addRow method. So removing the lastRowNum property is the best solution.

See also issue joomla#22690
@gaelicwinter
Copy link

I have tested this successfully but I get the message Bad Credentials when I try to mark the test result. However, it looks like it updated the test result anyways.

1 similar comment
@gaelicwinter
Copy link

I have tested this successfully but I get the message Bad Credentials when I try to mark the test result. However, it looks like it updated the test result anyways.

@Quy
Copy link
Contributor

Quy commented Nov 2, 2018

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/22757.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Nov 2, 2018
@mbabker mbabker added this to the Joomla 3.9.1 milestone Nov 5, 2018
@mbabker mbabker merged commit 4303714 into joomla:staging Nov 5, 2018
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Nov 5, 2018
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.

5 participants