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

[3] [com_users] Fix for fields output being loaded in edit form #28470

Closed
wants to merge 1 commit into from

Conversation

AndySDH
Copy link
Contributor

@AndySDH AndySDH commented Mar 26, 2020

Summary of Changes

Before this PR, the frontend "edit" layout (edit form) of the user profile (com_users), would also load in the background the fields output that is only meant to be displayed in the frontend result of the profile.

This PR makes sure the code to get the fields output is only loaded in the actual output profile page, not in the input edit form.

Before result

Loading the edit form is slower because it's actually loading the fields output as well, despite not using them

After result

Loading the edit form is faster because it doesn't load unnecessary data in the background

Documentation Changes Required

None

@AndySDH AndySDH changed the title [com_users] Fix for fields output being loaded in edit form [3] [com_users] Fix for fields output being loaded in edit form Mar 26, 2020
@HLeithner
Copy link
Member

I not sure if this is a good idea, if you have a template override or your own layout it will no longer load the fields...

@AndySDH
Copy link
Contributor Author

AndySDH commented Apr 2, 2020

Well then maybe it could be done as $this->getLayout() != 'edit' instead.

The problem is that the edit profile and the output profile are sharing the same view for some reason. Unlike com_content for example, which has separate views for article, form, etc. Here the edit form and the output are sharing the same view, just different layouts. This is a bad approach as it causes issues like this.

@HLeithner
Copy link
Member

And what exactly issue except that rendering the layout took longer?

@HLeithner
Copy link
Member

Do you have numbers? i mean we need it for default which normally is loaded before edit? We have the same performance impact on this screen.
Conceptional it maybe would better to load fields lazy when it's requested by the layout by this would effect every layout override using custom fields.

Conce

@AndySDH
Copy link
Contributor Author

AndySDH commented Apr 2, 2020

"i mean we need it for default which normally is loaded before edit"
What do you mean?

The issue is that the code that renders the fields output is loaded in the background also when you are requesting the edit form. With a lot of custom fields involved this will slow down the loading of the edit form page.

@HLeithner
Copy link
Member

I notice now that your PR has not much to do with custom fields.

I close this PR because you try to remove the possibility to prepare the content for user profiles and it doesn't matter if this is on the default page or the edit page. If a plugin prepares user profile output it's likely that it's also wants to change the edit page.

@HLeithner HLeithner closed this Apr 2, 2020
@AndySDH
Copy link
Contributor Author

AndySDH commented Apr 2, 2020

"If a plugin prepares user profile output it's likely that it's also wants to change the edit page"

What? What does that even mean? No, nothing of the profile output is used in the edit page at all

@HLeithner
Copy link
Member

But could be

@Hackwar
Copy link
Member

Hackwar commented Apr 2, 2020

FYI: Your PR would break several third party profile extensions and specifically 2 sites of customers of mine. We actually do need those events.

@AndySDH
Copy link
Contributor Author

AndySDH commented Apr 2, 2020

Ok then nevermind, can you please elaborate why they would break and why those events are needed? Curious to understand it.

@AndySDH
Copy link
Contributor Author

AndySDH commented Apr 2, 2020

@Hackwar

@AndySDH
Copy link
Contributor Author

AndySDH commented Nov 1, 2020

Got back to this issue, @Hackwar care to elaborate on the question above? :) Thanks!

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.

None yet

4 participants