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

[Bugfix] Too many embedded layers cause php to hit max_execution_time #4093

Merged
merged 2 commits into from
Feb 5, 2024

Conversation

mind84
Copy link
Contributor

@mind84 mind84 commented Jan 24, 2024

There are cases where a project might embed several layers from another project.

In this scenario Lizmap loads the same project (embedded project) multiple times for each of these layers and this behavior could cause php to hit max_execution_time if there are many embedded layers.

To fix that I've grouped the embedded layers and load the related QGIS embedded project once.

Test note
I haven't add php unit tests since seems to me that the current tests already covers this changes. Of course, if you think that more tests are needed, I'll add them.

Funded by Faunalia

@github-actions github-actions bot added this to the 3.8.0 milestone Jan 24, 2024
@mind84 mind84 changed the title [Bug] To many embedded layers cause php to hit max_execution_time [Bug] Too many embedded layers cause php to hit max_execution_time Jan 24, 2024
@Gustry
Copy link
Member

Gustry commented Jan 25, 2024

I guess it should be backported to 3.7 right ? 3.6 as well ?

@Gustry Gustry added run end2end If the PR must run end2end tests or not php Pull requests that update Php code labels Jan 25, 2024
@mind84
Copy link
Contributor Author

mind84 commented Jan 25, 2024

I guess it should be backported to 3.7 right ? 3.6 as well ?

I think 3.7. Don't know for 3.6

Thanks a lot

@mind84
Copy link
Contributor Author

mind84 commented Jan 25, 2024

I ran project_homepage e2e cypress test on a clean master branch and it fails at the same point.

        cy.get('.liz-project-title:contains("Test tags: nature, flower")')
            .prev('.liz-project')
            .children('.liz-project-desc').as('all-metadata') //<------- fails to get children('.liz-project-desc')

Should I update the test file too?

Seems not related to this pr.

@mind84
Copy link
Contributor Author

mind84 commented Jan 25, 2024

I ran project_homepage e2e cypress test on a clean master branch and it fails at the same point.

        cy.get('.liz-project-title:contains("Test tags: nature, flower")')
            .prev('.liz-project')
            .children('.liz-project-desc').as('all-metadata') //<------- fails to get children('.liz-project-desc')

Should I update the test file too?

Seems not related to this pr.

the same on this pr #4024

@Gustry
Copy link
Member

Gustry commented Jan 25, 2024

Should I update the test file too?

It's a work in progress already, you can skip this failing test.

@mind84 mind84 changed the title [Bug] Too many embedded layers cause php to hit max_execution_time [Bugfix] Too many embedded layers cause php to hit max_execution_time Jan 26, 2024
@Gustry Gustry added the sponsored development This development has been funded label Jan 29, 2024
@mind84
Copy link
Contributor Author

mind84 commented Jan 30, 2024

@Gustry,

can you please wait to merge this one? I've found another part that cause the same issue, pretty much the same behavior, and I would try to fix that part too.

@Gustry Gustry marked this pull request as draft January 30, 2024 11:03
@mind84 mind84 marked this pull request as ready for review January 30, 2024 15:50
@mind84 mind84 force-pushed the embedded_projects_loading branch 2 times, most recently from 46704be to 8a272a4 Compare January 31, 2024 08:30
@rldhont
Copy link
Collaborator

rldhont commented Jan 31, 2024

@mind84 you have requested to do not merged. You have marked the PR as Ready to review, yesterday. It is really ready to review and merged ?

@mind84
Copy link
Contributor Author

mind84 commented Jan 31, 2024

@mind84 you have requested to do not merged. You have marked the PR as Ready to review, yesterday. It is really ready to review and merged ?

Yes, I've done all changes, thanks

@nworr nworr self-requested a review January 31, 2024 11:27
@nworr
Copy link
Contributor

nworr commented Jan 31, 2024

i remove my approval, need to review the changes

@mind84 mind84 force-pushed the embedded_projects_loading branch 5 times, most recently from 7efff39 to 9ce7a9d Compare February 1, 2024 14:54
@Gustry
Copy link
Member

Gustry commented Feb 1, 2024

Let us know when this PR is finished, so the review can be done, we see you are still pushing on the branch

@mind84
Copy link
Contributor Author

mind84 commented Feb 1, 2024

Yes, I've done all changes, thanks

Hi @Gustry,

I've completed my changes yesterday, I'm just continue rebasing to let you merge the pr.
I think @nworr wants to review the code.

Thanks you

@mind84
Copy link
Contributor Author

mind84 commented Feb 1, 2024

The code is not changed since @rldhont approval

@Gustry
Copy link
Member

Gustry commented Feb 1, 2024

There a lot of merged PR today, do not worry for now about all these latest commits.
I though you were working on your PR and updating the code. We just have a notification saying your pull request has been updated, so not making the review easy.

Copy link
Collaborator

@mdouchin mdouchin left a comment

Choose a reason for hiding this comment

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

Thanks a lot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport release_3_7 php Pull requests that update Php code run end2end If the PR must run end2end tests or not sponsored development This development has been funded
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants