Conversation
760827b to
115c4e1
Compare
|
@kroky this is now working with Postale. Could you please review it when you have a chance? |
29f0f60 to
1a64c68
Compare
| $servers[$id] = $s; | ||
| } | ||
| } | ||
| SieveService::init($this->cache, $servers); |
There was a problem hiding this comment.
You can init once in a module and reuse the module whenever you need it.
7a54a6e to
6c368a4
Compare
1965bf7 to
069b879
Compare
ed606d9 to
b9d33c2
Compare
|
@christer77 @Baraka24 @IrAlfred I've implemented some enhancements on the block_list page. Please take some time to test them before @kroky proceeds with the review. |
|
Thanks @Shadow243 for this big work, we took the time to test differently and here are what we fund:
We expected that this should be coming from tls config along side
|
|
One more thing to note, please deal with caches, it is likely the updated block_list content are cached, to see updates we have to clear all caches, logout and login again the find the updates as well. Please check also as this a part of performance. |
Fixed 7a0bea3 |
|
@Shadow243 This branch has conflicts that must be resolved |
89223e3 to
d610249
Compare
@Baraka24 everything should work now. |
|
@Shadow243 Thanks. I tried and it worked. |
| * Service orchestrator - combines cache and connection management | ||
| * This provides backward compatibility and clean API | ||
| */ | ||
| class SieveService |
There was a problem hiding this comment.
You combine several very different classes in the same file which is quite confusing.
Also, the way we now use phpsievemanager is quite from obvious. It was purposefully designed as an external repo to handle sieve-related connection and interaction. Now we add so much sieve-related connection functionality directly in Cypht. I think we should add all that caching and changes to the php-sieve-manager repo and then keep cypht lightweight - just reuse.
Also, I don't like the fact that we got rid of the client factory - that is reused in Tiki and overridden there, so we will have even more changes to do.
In short, something simple to add like caching resulting in too much boilerplate here. I think we should greatly simplify the current approach. Add simple caching in php-sieve-manager. Bring back the factory and client usage. Add global modules that run on every sieve request rather than calls to methods like ensure_sieve_service_initialized in random list of modules. I don't even think we should have that method - the factory and client usage should be enough - it initializes once, keep cached version, return cached version when requested. No sieve module methods need changes in this case.
There was a problem hiding this comment.
Noted, I will push required changes.
0bd5410 to
b6e2313
Compare
c7451ab to
2f7d755
Compare
| $factory = get_sieve_client_factory($this->config); | ||
| try { | ||
| $client = $factory->init($this->user_config, $imap_account, $this->module_is_supported('nux')); | ||
| $client = get_sieve_client($imap_account, $this->config, $this->cache); |
There was a problem hiding this comment.
There seem to be too many changes in this MR not related to the purpose of reusing connections. Why do we get rid of the client factory? It was reused in Tiki and is a good pattern. Sometimes the client factory returns one class, sometimes another one. You could achieve the same result by modifying the client factory implementation is Cypht and reusing the connections via the cache. Why all these changes in the modules? I'd like to see a severely trimmed down version of a diff here just introducing the caching of connections change... note that you can cache the result of a client factory init and return the same client no matter how many times it is called...
2f7d755 to
74f5a9a
Compare
74f5a9a to
163a2ea
Compare
As discussed with @kroky via telegram, we aim to minimize the number of Sieve server reconnections to improve the block page performance.