Add repository layer to handlers for better and centralized entity and session management #625
Conversation
modules/smtp/modules.php
Outdated
| public function process() { | ||
| $servers = Hm_SMTP_List::dump(); | ||
| $servers = $this->generateId($servers); |
There was a problem hiding this comment.
This is good that we generate the IDs if we have cached server data from config. What about the rest of the lists - pop3, imap, profile, feed?
| } | ||
| } | ||
| if ($selected === false && $default !== false) { | ||
| $selected = $default; | ||
| } | ||
| foreach ($data['smtp_servers'] as $id => $vals) { | ||
| $smtp_profiles = profiles_by_smtp_id($profiles, $id); | ||
| $smtp_profiles = profiles_by_smtp_id($profiles, $vals['id']); |
There was a problem hiding this comment.
If we change the way smtp servers are references from an array key index to a generated ID, isn't it also better to update all the places where this index is used. I think profiles match imap to smtp servers by an index in some cases. Also, the smtp compose ID of the profile uses the index of the smtp server and the index of the profile while now it seems we should use the IDs of those 2...
|
@marclaporte @kroky There is an exaggerated complexity in ids and the way lists are stored and managed in cypht because there is no centralized way to do this, with the exception of servers, all modules decide how to store and manage their entities by themselves and save it in the session. My goal is to simplify the whole entity registration process (profiles, servers, keys, etc) as well as the search (getById, getByName, getByUsername and so on) of this entities. Repository Layer for ModulesAll registered and active modules will have an automatically initialized repository. This repository contains all the base resources needed to add, remove, edit and search for objects stored in the session.
Fully implemented Profile moduleI refactored the entire profiles module to work with this implementation and now I'm implementing it in the other modules. This will remove a lot of duplicate code. Below is the new code for inserting a profile and how it was before: BeforeAfter |
henrique-borba
changed the title
|
@henrique-borba yes, that will be great to implement this repository layout pattern! |
|
For anyone wondering: @henrique-borba is focusing on https://github.com/NumPower/numpower nowadays. |
kroky
force-pushed
the
fix/smtp_server_unique_ids
branch
from
417f069 to
5fbbef9
Compare
This update implements the 'id' field in SMTP Server objects and uses uniqid to generate unique ids for each server.
Include profiles implementation
… unique ID management to get rid of array index integer values mixing up when deleting entries
kroky
force-pushed
the
fix/smtp_server_unique_ids
branch
from
5fbbef9 to
ce62109
Compare
|
@marclaporte @josaphatim @Shadow243 This MR is ready. I'd love to get some of you test it out. Will also fix the Tiki integration once this is merged (need a couple of int/validation updates). I also have to see the automated tests but more or less this should be ready to be merged. |
|
Btw, one drawback of the switch is that existing config will still use integer IDs. We can make a migration script or try to auto-fix when loading data. The problem will be profiles that link one account to another by id - the profiles will need to be re-linked - i.e. set up again. Is this a big issue and should we think about an automated migration script? |
|
Some people use hundreds of profiles like here: #882 If we have a safe enough way to convert, let's do it |
|
These are some issues I found:
|
|
@josaphatim updated local contacts to use the repository pattern but had to do a bit of special cases where contacts are objects while all other entities are arrays and also we have external contacts in the same Hm_Contact_Store object that are not saved locally. Title bar issue seems to be a bootstrap migration issue which we can fix later. |
|
@marclaporte latest code here does an auto-migration when users load their config data - it replaces all integer IDs with unique IDs and fixes the links between each other. |
An MR to fix once and for all the problem of ids in the cypht, in addition to facilitating all the management and access of entities in each module.
Repository Layer for Modules
All registered and active modules will have an automatically initialized repository. This repository contains all the base resources needed to add, remove, edit and search for objects stored in the session.
https://github.com/jasonmunro/cypht/blob/9b3e9f7236fd0d11b8b6d3c27f1215bc22d46e6b/modules/profiles/modules.php#L130
Fully implemented Profile module
I refactored the entire profiles module to work with this implementation and now I'm implementing it in the other modules. This will remove a lot of duplicate code.
Below is the new code for inserting a profile and how it was before:
Before
https://github.com/jasonmunro/cypht/blob/07605067dbc996c0f5d9e5ffe512c36aa4f85886/modules/profiles/modules.php#L117-L147
After
https://github.com/jasonmunro/cypht/blob/9c001c33799e806474b253c7ebdc757806c35279/modules/profiles/modules.php#L118-L137