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

Added auto-rebuild form if internal cache fails #240

Merged
merged 2 commits into from
Mar 9, 2018

Conversation

BasileTrujillo
Copy link
Contributor

This pull request aim to add an auto form re-build process in the case of the internal form cache has been expired before the global pages grav cache.

Indeed if Grav hit the cache of the core Pages object, it will not trigger the onPageProcessed event (which is listen by the form plugin to build forms). So in this case, if the form cache expires before the Grav core Pages cache (yes it sometime happen, but I dont know why or when ^_^), then the form plugin (listing onPageInitialized) will get an empty form (because it was not built through the onPageProcessed) and an empty cache (because it was expired) and result to a completely broken form (no fields, no submit...).

I don't know if it's clear but this fix is in testing on my side in some live website and seems to correct the initial issue (forms sometime just disappears, and a manual clear cache solve the problem).

I can re-write this speech if it is not truly understandable (I am a french guy and know that my english speaking is not perfect ;) )

@rhukster
Copy link
Member

I really cannot replicate this error. I have turned on caching in my test setup and set the timeout for 30secs (just for the sake of it), then when i got to a page with a form, wait 30 secs, then reload the form is auto-rebuilt (as part of onPageProcessed() event) and works as expected.

Can you please outline how you run into issues? Also your solution will only update the form for the current page, so it won't solve issues with forms that are pulled from other pages etc. I would like to be able to replicate this issue, then come up with a more complete solution.

Cheers!

@rhukster
Copy link
Member

Ok found something.. investigating...

@rhukster
Copy link
Member

So I found an issue, and not sure if it's related to what you are seeing, but its basically due to the recent refactor moving the caching logic into onPageInitialized rather than onPagesInitialized.

This occurs when you try to use Twig based form injection in page content. It simply wasn't working when the cache was enabled. I've fixed with this commit:

0d6ba7c

You might want to test with latest Grav develop branch to see if it resolves the issue you are seeing.

@BasileTrujillo BasileTrujillo force-pushed the feature/prevent-cache-issue branch 2 times, most recently from 6ff296c to 3b9b68d Compare February 22, 2018 09:56
@BasileTrujillo
Copy link
Contributor Author

Great refacto 👍 !

But unfortunately it does not solve the issue when form cache expires before the Grav page cache.
Actually since I don't understand why this happen, to reproduce it I manually comment the code of the loadCachedForms() function to reproduce this odd behavior.

By the way, the original issue was that my forms (all) sometimes disappears on my production websites (sometime after a change in admin, but not all the time, and sometime without any change), and the only way to get them back is by clearing the grav cache.

Note: Indeed I use Twig based form injection in page content with cache_enable: false but grav still don't want to trigger the onPageProcessed for a page with the cache disabled...

@rhukster rhukster merged commit 26b6c2d into getgrav:develop Mar 9, 2018
@BasileTrujillo BasileTrujillo deleted the feature/prevent-cache-issue branch March 13, 2018 14:04
@mahagr
Copy link
Member

mahagr commented Apr 17, 2018

Unfortunately this pull request didn't really fix the issue -- now all forms except the one in the current page will be broken. So if the form cache expires, form in the current page will be ok, but every other form in the whole site will break.

@mahagr
Copy link
Member

mahagr commented Apr 17, 2018

@BasileTrujillo @rhukster Above workaround will improve the situation, but will not fully fix it.

My change prevents form cache from being saved when the cache fails, thus making most of the forms to work. Unfortunately, it doesn't fix the issue with forms that come from other pages.

A better fix is needed, but at least most common forms will now work with caching.

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

3 participants