8000 [HttpFoundation] Allow dynamic session "ttl" when using a remote storage by nicolas-grekas · Pull Request #45873 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[HttpFoundation] Allow dynamic session "ttl" when using a remote storage #45873

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 30, 2022
Merged

[HttpFoundation] Allow dynamic session "ttl" when using a remote storage #45873

merged 1 commit into from
Mar 30, 2022

Conversation

nicolas-grekas
Copy link
Member
@nicolas-grekas nicolas-grekas commented Mar 28, 2022 •
edited
Loading
Q A
Branch? 6.1
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #44368
License MIT
Doc PR -

This is my proposal instead of #44368. My goal here it to not make the session handler mutable.
Instead, I propose to allow defining the "ttl" option as a closure.
Here is how this would work in practice:

services:
  my.session.handler:
    parent: session.abstract_handler
    arguments:
      index_0: 'redis://localhost'
      index_1: {ttl: !closure ['@my.ttl.handler']}

Where my.ttl.handler would be an invokable that returns the ttl for the current user (based on the session object from the request stack, or from the token storage I suppose).

/cc @Seldaek I'd be happy to have your thoughts here (and also your help for tests/doc ideally 👼)

@carsonbot carsonbot added this to the 6.1 milestone Mar 28, 2022
@nicolas-grekas nicolas-grekas force-pushed the hf-ttl-closure branch 4 times, most recently from 7263e63 to 463a441 Compare March 28, 2022 19:04
@Seldaek Seldaek mentioned this pull request Mar 29, 2022
Closed
@nicolas-grekas nicolas-grekas mentioned this pull request Mar 29, 2022
Merged
@nicolas-grekas
Copy link
Member Author

PR is ready with basic tests.

chalasr
chalasr approved these changes Mar 29, 2022
View reviewed changes
Copy link
Member
@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

Nice. Worth some changelog entry though

fabpot added a commit that referenced this pull request Mar 30, 2022
@fabpot
feature #45878 [DependencyInjection] Add argument type closure to h…
7cdf8b4 
…elp passing closures to services (nicolas-grekas)

This PR was merged into the 6.1 branch.

Discussion
----------

[DependencyInjection] Add argument type `closure` to help passing closures to services

| Q             | A
| ------------- | ---
| Branch?       | 6.1
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

Inspired by #45873

This makes it easier to inject invokable services as closures.

Before:
```yaml
services:
  # [...]
    arguments:
      - !service
          class: Closure
          factory: [Closure, fromCallable]
          arguments: ['@my.invokable']
```

After:
```yaml
services:
  # [...]
    arguments:
      - !closure '@my.invokable'
```

As a bonus, PhpDumper now optimizes Closure::fromCallable into first-class callable syntax.

Commits
-------

fe1404f [DependencyInjection] Add argument type `closure` to help passing closures to services
fabpot
fabpot approved these changes Mar 30, 2022
View reviewed changes
Copy link
Member
@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

I agree that it deserves a note in the CHANGELOG file.

@nicolas-grekas
Copy link
Member Author

changelog updated

@fabpot
Copy link
Member
fabpot commented Mar 30, 2022

Thank you @nicolas-grekas.

@fabpot fabpot mentioned this pull request Mar 30, 2022
Closed
@fabpot fabpot merged commit acee03f into symfony:6.1 Mar 30, 2022
@nicolas-grekas nicolas-grekas deleted the hf-ttl-closure branch March 31, 2022 13:33
@fabpot fabpot mentioned this pull request Apr 15, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@Seldaek Seldaek Seldaek approved these changes

@chalasr chalasr chalasr approved these changes

Assignees
No one assigned
Labels
Feature HttpFoundation Status: Reviewed
Projects
None yet
Milestone
6.1
Development

Successfully merging this pull request may close these issues.
5 participants
@nicolas-grekas @fabpot @Seldaek @chalasr @carsonbot
0