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

[Associations] Correcting Copy Reference to Target issue #16178

Merged
merged 1 commit into from May 22, 2017

Conversation

Projects
None yet
7 participants
@infograf768
Member

infograf768 commented May 22, 2017

Pull Request for Issue #16168

Summary of Changes

src is used elsewhere in the js.
Therefore the error was to use data-url

Testing Instructions

Patch. Clear all caches.

After Patch

All should be fine.

multiassoc_copy

@appnweb @brianteeman @PhilETaylor @AlexRed @alikon

@AlexRed

This comment has been minimized.

Show comment
Hide comment
@AlexRed

AlexRed May 22, 2017

Contributor

I have tested this item successfully on 788f99c

Patch ok for me


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

Contributor

AlexRed commented May 22, 2017

I have tested this item successfully on 788f99c

Patch ok for me


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

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman May 22, 2017

Contributor

I have tested this item successfully on 788f99c


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

Contributor

brianteeman commented May 22, 2017

I have tested this item successfully on 788f99c


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

@rdeutz rdeutz merged commit 4830082 into joomla:staging May 22, 2017

4 checks passed

JTracker/HumanTestResults Human Test Results: 2 Successful 0 Failed.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rdeutz rdeutz removed the release-blocker label May 22, 2017

@infograf768 infograf768 deleted the infograf768:multiassoc_copy branch May 22, 2017

@PhilETaylor

This comment has been minimized.

Show comment
Hide comment
@PhilETaylor

PhilETaylor May 22, 2017

Contributor

Did everyone also test to see if this still fixes the race condition?

Im not convinced that this is the correct fix, all this does is revert my race condition fix, and put a iframe reload in place, the race condition would still be present, but masked.

Contributor

PhilETaylor commented May 22, 2017

Did everyone also test to see if this still fixes the race condition?

Im not convinced that this is the correct fix, all this does is revert my race condition fix, and put a iframe reload in place, the race condition would still be present, but masked.

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman May 22, 2017

Contributor

No i didnt as I couldnt replicate that before

Contributor

brianteeman commented May 22, 2017

No i didnt as I couldnt replicate that before

@infograf768

This comment has been minimized.

Show comment
Hide comment
@infograf768

infograf768 May 22, 2017

Member

I did not get any race condition anymore ( You are not permitted, etc.)

Member

infograf768 commented May 22, 2017

I did not get any race condition anymore ( You are not permitted, etc.)

@dgrammatiko

This comment has been minimized.

Show comment
Hide comment
@dgrammatiko

dgrammatiko May 22, 2017

Member

@PhilETaylor you can still have your code back here with a little more code, e.g. have a container div with all the attributes the iframe needs and then create the iframe element in runtime. That will cover the race conditions, I guess

Member

dgrammatiko commented May 22, 2017

@PhilETaylor you can still have your code back here with a little more code, e.g. have a container div with all the attributes the iframe needs and then create the iframe element in runtime. That will cover the race conditions, I guess

@infograf768

This comment has been minimized.

Show comment
Hide comment
@infograf768

infograf768 May 22, 2017

Member

If a variable is changed —in this case the variable src was changed to data-url in the edit form, then it should have been changed all over the js when the target is loaded. That was not the case and it indeed broke the target reload.

Member

infograf768 commented May 22, 2017

If a variable is changed —in this case the variable src was changed to data-url in the edit form, then it should have been changed all over the js when the target is loaded. That was not the case and it indeed broke the target reload.

@PhilETaylor

This comment has been minimized.

Show comment
Hide comment
@PhilETaylor

PhilETaylor May 22, 2017

Contributor

If it works it works.

Contributor

PhilETaylor commented May 22, 2017

If it works it works.

@PhilETaylor

This comment has been minimized.

Show comment
Hide comment
@PhilETaylor

PhilETaylor May 22, 2017

Contributor

The copy to reference is a huge crap load of functions, it first saves the article in the left pane, then loads an article in the left pane, then decides its the wrong language and so loads the initial language in the left pane, and then finally loads the right pane....

The whole component is badly written - because it tried to reuse existing core features (like tmpl=component)...

So if this fixes the race and fixes the copy then excellent...

Contributor

PhilETaylor commented May 22, 2017

The copy to reference is a huge crap load of functions, it first saves the article in the left pane, then loads an article in the left pane, then decides its the wrong language and so loads the initial language in the left pane, and then finally loads the right pane....

The whole component is badly written - because it tried to reuse existing core features (like tmpl=component)...

So if this fixes the race and fixes the copy then excellent...

@infograf768

This comment has been minimized.

Show comment
Hide comment
@infograf768

infograf768 May 22, 2017

Member

I hope someone will improve all this in 4.0 😺

Member

infograf768 commented May 22, 2017

I hope someone will improve all this in 4.0 😺

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment