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

Lib framework code optimizations #948

Merged

Conversation

TheDigitalOrchard
Copy link
Contributor

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

  • None

Checklist

  • None

How2Test

  • None

Todo

  • None

… more closely adhere to PSR-12; changed array initializers to short-array (square bracket) syntax;
@TheDigitalOrchard
Copy link
Contributor Author

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 marclaporte requested a review from Shadow243 April 13, 2024 22:52
@marclaporte
Copy link
Member

Thank you @TheDigitalOrchard

@Shadow243 can you please review?

Copy link
Member

@Shadow243 Shadow243 left a comment

Choose a reason for hiding this comment

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

Hooray 🎉, this is a cleaning refactoring!

Replacing array_key_exists() with just empty() might introduce a potential issue where accessing a non-existent key in an array could trigger a notice-level error in PHP. This is because empty() will attempt to access the key directly without first checking if it exists, leading to an undefined offset error if the key is not present. Therefore, it's essential to ensure the key exists using isset() or array_key_exists() before using empty() to avoid generating notice-level errors.

lib/auth.php Show resolved Hide resolved
lib/auth.php Show resolved Hide resolved
lib/auth.php Show resolved Hide resolved
lib/framework.php Outdated Show resolved Hide resolved
lib/session_base.php Show resolved Hide resolved
@josaphatim josaphatim closed this Apr 14, 2024
@josaphatim josaphatim reopened this Apr 14, 2024
@marclaporte
Copy link
Member

(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 marclaporte requested a review from kroky April 19, 2024 11:20
@marclaporte
Copy link
Member

@TheDigitalOrchard please address conflicts.

@marclaporte
Copy link
Member

Cypht 1.4.0 was released in July 2023. We will release Cypht 2.0 any day now.
#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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

@TheDigitalOrchard
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

kroky commented Apr 29, 2024

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

@TheDigitalOrchard
Copy link
Contributor Author

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.

…ments intead of three separate `if` statements
@TheDigitalOrchard
Copy link
Contributor Author

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
Copy link
Member

kroky commented Apr 30, 2024

@TheDigitalOrchard looks good, thanks!

@kroky kroky merged commit 8fbd655 into cypht-org:master Apr 30, 2024
6 checks passed
@marclaporte
Copy link
Member

@TheDigitalOrchard please see: #999

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.

5 participants