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

Minz: Attach a notification to a request #3208

Merged
merged 6 commits into from
Oct 5, 2020

Conversation

Alkarex
Copy link
Member

@Alkarex Alkarex commented Oct 3, 2020

Notifications should be attached to a request, not to a global session.
Implement #3096 (comment)
Prepare #3096

Notifications should be attached to a request, not to a global session.
Fix
FreshRSS#3096 (comment)
Prepare FreshRSS#3096
@Alkarex Alkarex marked this pull request as draft October 3, 2020 21:52
@Alkarex Alkarex added this to the 1.18.0 milestone Oct 3, 2020
@Alkarex Alkarex marked this pull request as ready for review October 3, 2020 21:53
lib/Minz/Request.php Outdated Show resolved Hide resolved
lib/Minz/Request.php Outdated Show resolved Hide resolved
@aledeg
Copy link
Member

aledeg commented Oct 4, 2020

I like what you've done here.

I was wondering if it could be a good idea to have more than one setter for that matter. Let me explain!
Instead of using Minz_Request::setNotification('good', _t('feedback.conf.updated')); use Minz_Request::setSuccessNotification(_t('feedback.conf.updated'));. Instead of using Minz_Request::setNotification('bad', $msg); use Minz_Request::setFailureNotification($msg);

This way, there is no need to rely on a constant value everywhere in the codebase, which is prone to errors. I've used the words success and failure in the method names but it could message levels like in monolog.
Using such thing will make the code more expressive and easier to check automatically.

@Alkarex
Copy link
Member Author

Alkarex commented Oct 4, 2020

Good idea, here you are @aledeg 24b846b

@Alkarex
Copy link
Member Author

Alkarex commented Oct 4, 2020

@marienfressinaud Just for the record, if this Minz codebase is used in any other context, the former way of setting notifications is still possible Minz_View::_param('notification', [ ... ])

app/Controllers/userController.php Show resolved Hide resolved
lib/Minz/Request.php Show resolved Hide resolved
lib/Minz/Request.php Outdated Show resolved Hide resolved
@marienfressinaud
Copy link
Member

Just for the record, if this Minz codebase is used in any other context […]

I hope it isn't haha 😄

@Alkarex Alkarex merged commit 7652369 into FreshRSS:master Oct 5, 2020
@Alkarex Alkarex deleted the minz_notifications branch October 5, 2020 17:03
Alkarex added a commit to Alkarex/FreshRSS that referenced this pull request Oct 5, 2020
Implement mutex for notifications
FreshRSS#3208 (comment)
Alkarex added a commit that referenced this pull request Oct 6, 2020
* Minz allow parallel sessions

#fix #3093

* Array optimisation

* Array optimisation missing

* Reduce direct access to $_SESSION except in install process

* Fix session start headers warning

* Use cookie only the first time the session is started:
`PHP Warning:  session_start(): Cannot start session when headers
already sent in /var/www/FreshRSS/lib/Minz/Session.php on line 39`

* New concept of volatile session for API calls

Optimisation: do not use cookies or local storage at all for API calls
without a Web session
Fix warning:

```
PHP Warning:  session_destroy(): Trying to destroy uninitialized session
in Unknown on line 0
```

* Only call Minz_Session::init once in our index

It was called twice (once indirectly via FreshRSS->init())

* Whitespace

* Mutex for notifications

Implement mutex for notifications
#3208 (comment)

* Typo

* Install script is not ready for using Minz_Session
Alkarex added a commit to Alkarex/FreshRSS that referenced this pull request Dec 29, 2020
Caused by FreshRSS#3208
Before, the 'rid' parameter was saved as part of the user query.
Alkarex added a commit that referenced this pull request Dec 29, 2020
Caused by #3208
Before, the 'rid' parameter was saved as part of the user query.
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.

3 participants