cypht-org / cypht

Cypht: Lightweight Open Source webmail aggregator [PHP, JS]
http://cypht.org
GNU Lesser General Public License v2.1
949 stars 147 forks source link

Lib framework code optimizations #948

Closed TheDigitalOrchard closed 2 months ago

TheDigitalOrchard commented 2 months ago

Pullrequest

No functional changes. Code cleanup, whitespace; short-array syntax; single quotes; closer adherence to PSR-12 syntax guidelines.

Now, I understand there may be some clashing with personal preferences, such as how "else" begins on the next line, but I did see some of the newer files already adhering to PSR-12, so figured this was a safe change to make.

(Sidenote, Packagist.org still says this library is compatible back to PHP 5, but that doesn't appear to be true anymore.)

Issues

Checklist

How2Test

Todo

TheDigitalOrchard commented 2 months ago

One more thing that I changed ... there was a heavy usage of array_key_exists() followed by a truthy check on that value. Replacing this with an empty() or !empty() call achieves the same thing with less code.

marclaporte commented 2 months ago

Thank you @TheDigitalOrchard

@Shadow243 can you please review?

marclaporte commented 2 months ago

(Sidenote, Packagist.org still says this library is compatible back to PHP 5, but that doesn't appear to be true anymore.)

Master (from which we will soon branch 2.x) requires PHP 7.4: https://github.com/cypht-org/cypht/blob/master/composer.json

Stable branch Cypht 1.4.x requires PHP 5.6: https://github.com/cypht-org/cypht/blob/1.4.x/composer.json

marclaporte commented 2 months ago

@TheDigitalOrchard please address conflicts.

marclaporte commented 2 months ago

Cypht 1.4.0 was released in July 2023. We will release Cypht 2.0 any day now. https://github.com/cypht-org/cypht/issues/879

It will be one of the largest releases in Cypht's history, with numerous changes and enhancements. Most are incremental (just adding an optional feature) but some are disruptive (like moving to Bootstrap 5, revamp of account creation, etc.) which is why it's Cypht 2.0 and not 1.5.0. Furthermore, many of these changes were contributed by developers that are new to the project. A lot of big changes + new developers = I expect a lot of bugs.

We have many developers active, and things will stabilize nicely in Cypht 2.1, 2.2, etc. The process is that each bug will be fixed in master, and then backported to the 2.x branch. Backporting becomes more difficult as 2.x and master diverge. This cleanup PR affects a lot of files. It would be really good to get this in before 2.0. If not, we should at least backport to the 2.x branch when the time comes.

TheDigitalOrchard commented 2 months ago

@kroky Are these conflicts still present? I'm a bit lost here. Not seeing them and everything is pushed. But I can't see how to mark them as resolved here. What am I missing?

TheDigitalOrchard commented 2 months ago

@kroky Figured it out. Forgot to sync and pull the master branch. Once I did that, I saw the conflicts and resolved them.

TheDigitalOrchard commented 2 months ago

We have many developers active, and things will stabilize nicely in Cypht 2.1, 2.2, etc. The process is that each bug will be fixed in master, and then backported to the 2.x branch. Backporting becomes more difficult as 2.x and master diverge. This cleanup PR affects a lot of files. It would be really good to get this in before 2.0. If not, we should at least backport to the 2.x branch when the time comes.

Thanks for the details @marclaporte. On that note, I'm not seeing a 2.x branch. Is it private? Is development for v2.x being done on master? A bit confused by this. I've been keeping my pull requests in sync with master. Let me know if I'm on the right track here. Should we have a separate 2.x branch for clarity, or a dev branch?

marclaporte commented 2 months ago

Is development for v2.x being done on master

Yes. @kroky will create the branch soon, along with a release of 2.0 alpha.

https://github.com/cypht-org/cypht/wiki/Lifecycle

kroky commented 2 months ago

@TheDigitalOrchard I don't see any conflicts. We can merge but we should resolve first the conversation about libsodium...

TheDigitalOrchard commented 2 months ago

Yes, re-reading the original sodium logic shows that I was a bit too hasty with the refactoring. I missed the fact that multiple extensions (libsodium and sodium) were being checked, and one of two separate classes (Hm_Sodium_PECL and Hm_Sodium_PHP) were being extended.

Will push a fix. That may involve reverting to the original code.

TheDigitalOrchard commented 2 months ago

Original libsodium logic check restored, but the only change being correct if/else branching instead of three separate if statements as was being done before. That avoids the redundant calls to defined('LIBSODIUM').

kroky commented 2 months ago

@TheDigitalOrchard looks good, thanks!

marclaporte commented 2 months ago

@TheDigitalOrchard please see: https://github.com/cypht-org/cypht/issues/999