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

New notifer for Linux which use libnotify and PHP-FFI #100

Merged
merged 4 commits into from
Apr 29, 2024
Merged

New notifer for Linux which use libnotify and PHP-FFI #100

merged 4 commits into from
Apr 29, 2024

Conversation

morawskim
Copy link
Contributor

Hello

This PR add another notifier, which use FFI and libnotify.
Probably most of Linux notifier supported in this repository under the hood use this library.

This notifier has some advantages and disadvantages.
Support for FFI in PHP vary between distributions. Sometimes we need to install dedicated package.
In other hand can be slightly faster (we don't need to crate external process)
and more portable (we don't need external application, only libnotify).

FFI allows us to call C code from PHP script.
The FFI extension need to be installed to use this notifier.
Also support for FFI is only in CLI SAPI, due to security concerns.

At the moment I tested this notifier with:

  • Ubuntu 20
  • Ubuntu 22
  • Ubuntu 24
  • Fedora 39
  • OpenSUSE Tumbleweed

All existing notifiers use external commands and maybe this new notifier does not meet project goals.
I decided to mark this PR as draft.
I can finish this PR if that notifier can be included in this package, otherwise you can close this PR.

@pyrech
Copy link
Member

pyrech commented Mar 17, 2024

Hi @morawskim.

Thanks for your PR, it looks realy good. I will need to play a bit a with it but this is definitely something that could bin included in JoliNotif

@pyrech
Copy link
Member

pyrech commented Apr 24, 2024

Hi @morawskim Do you have some time to finish this PR? It would be great to support a FFI-based notifier for linux environments.

@morawskim
Copy link
Contributor Author

Yes, I will finish this PR.
I should do this at the weekend.

@morawskim
Copy link
Contributor Author

Hello @pyrech

I wanted to write some additional unit tests, but FFI class is final, so we cannot create mock class.
I could create some FFI Wrapper and inject this in constructor for tests, but IMHO this would not give any value.
If you have some ideas, I am very open for discussion.

I checked some potential memory leaks lastly. Valgrind report some memory leak, but not in libnotify.
Also after running 400 notifications I didn't see any issues with memory.

@morawskim morawskim marked this pull request as ready for review April 27, 2024 18:21
@pyrech
Copy link
Member

pyrech commented Apr 28, 2024

Thanks for your work so far. About the tests, I understand it's not ideal to unit test the FFI part. Instead maybe we could just trigger the notifier for real and ensure there is no exception (and skip the tests when the notifier is not available). It would be better than nothing. What do you think about it?

@morawskim
Copy link
Contributor Author

Thanks for suggestion @pyrech

Instead maybe we could just trigger the notifier for real and ensure there is no exception (and skip the tests when the notifier is not available).

I have considered this, but I was afraid that this will require a lots of dependencies and the image will be huge.

But now....
I dig deeper and we can install only libnotify4 package in Ubuntu without any recommends and suggestions.

Simple test:
Run container - docker run --rm -it -e PHP_EXTENSIONS=ffi -v $PWD:/usr/src/app thecodingmachine/php:8.3-v4-cli bash
In container install only libnotify - sudo apt-get update -y && sudo apt-get install -y --no-install-recommends --no-install-suggests libnotify4
And in output you will see:

After this operation, 3470 kB of additional disk space will be used.

Amazing, we don't need install desktop environment, only one library.
I added test for initialize the FFI.
I have also updated GitHub workflow.

Runt tests ./vendor/bin/simple-phpunit tests/Notifier/LibNotifyNotifierTest.php

morawskim and others added 3 commits April 29, 2024 13:56

Verified

This commit was signed with the committer’s verified signature.
pyrech Loïck Piera

Verified

This commit was signed with the committer’s verified signature.
pyrech Loïck Piera

Verified

This commit was signed with the committer’s verified signature.
pyrech Loïck Piera
@pyrech
Copy link
Member

pyrech commented Apr 29, 2024

I just rebased your PR in order to have PHPStan running in the CI. I also:

  • added a note in the Changelog
  • added a test testSendNotificationWithAllOptions that really triggers FFI and libnotify in the CI (even if the notifier returns false but I think that's expected since no desktop is installed on the CI, so I skip the test in this case but it works on my computer).

It worked out of the box on my computer, so congrats for your PR.

@pyrech pyrech merged commit 3647c3c into jolicode:main Apr 29, 2024
6 checks passed
@pyrech
Copy link
Member

pyrech commented Apr 29, 2024

Thanks a lot for your contribution @morawskim 🙏

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