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

Register PdoSessionHandlerSchemaSubscriber #1569

Conversation

alli83
Copy link
Contributor

@alli83 alli83 commented Oct 31, 2022

@alli83 alli83 marked this pull request as draft October 31, 2022 12:41
@alli83 alli83 force-pushed the doctrine-bundle-add-config-pdo-session-handler-schema-subscriber branch 2 times, most recently from 4681eb8 to 7a00e16 Compare October 31, 2022 13:29
@alli83 alli83 marked this pull request as ready for review October 31, 2022 13:32
Copy link
Member

@ostrolucky ostrolucky left a comment

Choose a reason for hiding this comment

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

LGTM, but should go to 2.8.x. And should be merged only after upstream PR is merged.

@ostrolucky ostrolucky changed the base branch from 2.7.x to 2.8.x October 31, 2022 14:09
@alli83 alli83 force-pushed the doctrine-bundle-add-config-pdo-session-handler-schema-subscriber branch from 7a00e16 to 2186954 Compare October 31, 2022 20:35
@nicolas-grekas nicolas-grekas force-pushed the doctrine-bundle-add-config-pdo-session-handler-schema-subscriber branch from 2186954 to 133aeb0 Compare November 7, 2022 16:05
@nicolas-grekas nicolas-grekas changed the title [DoctrineBundle] feat: add config PdoSessionHandlerSchemaSubscriber Register PdoSessionHandlerSchemaSubscriber Nov 7, 2022
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Thanks for this PR @alli83
I force-pushed on your fork to sync with changes I made on symfony/symfony#48059
Can you please have a look at CI failures, once my comment is resolved?
🙏

@alli83
Copy link
Contributor Author

alli83 commented Nov 8, 2022

Thanks for this PR @alli83
I force-pushed on your fork to sync with changes I made on symfony/symfony#48059
Can you please have a look at CI failures, once my comment is resolved?
🙏

Thanks. yes, of course!

@ostrolucky ostrolucky added the Status: On Hold Most likely waiting for upstream resolution label Nov 8, 2022
nicolas-grekas added a commit to symfony/symfony that referenced this pull request Dec 16, 2022
…en pdo handler is used (alli83)

This PR was squashed before being merged into the 6.3 branch.

Discussion
----------

[HttpFoundation] Create migration for session table when pdo handler is used

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

The purpose of this PR is to automatically generate the session table with the make migration command in addition to https://symfony.com/doc/current/session/database.html#preparing-the-database-to-store-sessions

Even though  `WellKnownSchemaFilterPass`  is deprecated, atm the session table is blacklisted by the `WellKnownSchemaFilterPass`, that's why I need to set the SchemaAssetFilter to null.

todo:

- [ ] doctrineBundle => doctrine/DoctrineBundle#1569
- [x] Add mention to changelog
- [ ] update documentation

Commits
-------

aebdc58 [HttpFoundation] Create migration for session table when pdo handler is used
symfony-splitter pushed a commit to symfony/http-foundation that referenced this pull request Dec 16, 2022
…en pdo handler is used (alli83)

This PR was squashed before being merged into the 6.3 branch.

Discussion
----------

[HttpFoundation] Create migration for session table when pdo handler is used

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

The purpose of this PR is to automatically generate the session table with the make migration command in addition to https://symfony.com/doc/current/session/database.html#preparing-the-database-to-store-sessions

Even though  `WellKnownSchemaFilterPass`  is deprecated, atm the session table is blacklisted by the `WellKnownSchemaFilterPass`, that's why I need to set the SchemaAssetFilter to null.

todo:

- [ ] doctrineBundle => doctrine/DoctrineBundle#1569
- [x] Add mention to changelog
- [ ] update documentation

Commits
-------

aebdc581fa [HttpFoundation] Create migration for session table when pdo handler is used
symfony-splitter pushed a commit to symfony/cache that referenced this pull request Dec 16, 2022
…en pdo handler is used (alli83)

This PR was squashed before being merged into the 6.3 branch.

Discussion
----------

[HttpFoundation] Create migration for session table when pdo handler is used

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

The purpose of this PR is to automatically generate the session table with the make migration command in addition to https://symfony.com/doc/current/session/database.html#preparing-the-database-to-store-sessions

Even though  `WellKnownSchemaFilterPass`  is deprecated, atm the session table is blacklisted by the `WellKnownSchemaFilterPass`, that's why I need to set the SchemaAssetFilter to null.

todo:

- [ ] doctrineBundle => doctrine/DoctrineBundle#1569
- [x] Add mention to changelog
- [ ] update documentation

Commits
-------

aebdc581fa [HttpFoundation] Create migration for session table when pdo handler is used
symfony-splitter pushed a commit to symfony/doctrine-bridge that referenced this pull request Dec 16, 2022
…en pdo handler is used (alli83)

This PR was squashed before being merged into the 6.3 branch.

Discussion
----------

[HttpFoundation] Create migration for session table when pdo handler is used

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

The purpose of this PR is to automatically generate the session table with the make migration command in addition to https://symfony.com/doc/current/session/database.html#preparing-the-database-to-store-sessions

Even though  `WellKnownSchemaFilterPass`  is deprecated, atm the session table is blacklisted by the `WellKnownSchemaFilterPass`, that's why I need to set the SchemaAssetFilter to null.

todo:

- [ ] doctrineBundle => doctrine/DoctrineBundle#1569
- [x] Add mention to changelog
- [ ] update documentation

Commits
-------

aebdc581fa [HttpFoundation] Create migration for session table when pdo handler is used
@nicolas-grekas
Copy link
Member

Upsteam is now merged :)

@alli83 alli83 force-pushed the doctrine-bundle-add-config-pdo-session-handler-schema-subscriber branch 2 times, most recently from 97e4d55 to ac6361b Compare December 20, 2022 07:36
@ostrolucky ostrolucky self-requested a review December 20, 2022 09:06
@ostrolucky ostrolucky removed the Status: On Hold Most likely waiting for upstream resolution label Dec 20, 2022
@alli83 alli83 force-pushed the doctrine-bundle-add-config-pdo-session-handler-schema-subscriber branch 2 times, most recently from ebcb76d to 930a708 Compare December 20, 2022 09:11
@alli83 alli83 force-pushed the doctrine-bundle-add-config-pdo-session-handler-schema-subscriber branch 2 times, most recently from 29c077f to 9699326 Compare December 21, 2022 17:00
@alli83 alli83 force-pushed the doctrine-bundle-add-config-pdo-session-handler-schema-subscriber branch from 9699326 to 779da5a Compare December 21, 2022 17:03
@ostrolucky ostrolucky added this to the 2.8.0 milestone Dec 21, 2022
@derrabus derrabus added the Status: On Hold Most likely waiting for upstream resolution label Dec 28, 2022
@derrabus
Copy link
Member

I this ready to be merged or do we wait for the 6.3 release?

@derrabus derrabus modified the milestones: 2.8.0, 2.9.0 Dec 29, 2022
@chalasr
Copy link
Contributor

chalasr commented Dec 29, 2022

It seems safe to merge it given the conditional removal of the definition

@alli83
Copy link
Contributor Author

alli83 commented Dec 29, 2022

I was thinking of maybe adding some documentation but I don't think it belongs here so I will make an other PR on symfony-docs repo instead

@derrabus
Copy link
Member

I was thinking of maybe adding some documentation but I don't think it belongs here so I will make an other PR on symfony-docs repo instead

Documentation regarding this bundle is maintain in this repository. Don't hesitate to add it to the PR.

@alli83
Copy link
Contributor Author

alli83 commented Dec 29, 2022

I was thinking of maybe adding some documentation but I don't think it belongs here so I will make an other PR on symfony-docs repo instead

Documentation regarding this bundle is maintain in this repository. Don't hesitate to add it to the PR.

What I don't understand (if it's possible to enlighten me) is that the changes made on the WellKnownSchemaFilterPass is more of an "internal operation" that will depend on what has been done on the PdoSessionHandlerSchemaSubscriber and consequently on the PdoSessionHandler Class in the Symfony repo. I'm having a little trouble understanding the point of the documentation at this level.
Maybe I'm wrong but for example the fact that the session table for a relational database can be generated automatically with make:migration doesn't seem to have a place here.
Thanks for the help !

@ostrolucky ostrolucky removed the Status: On Hold Most likely waiting for upstream resolution label Dec 30, 2022
@ostrolucky ostrolucky modified the milestones: 2.9.0, 2.8.1 Dec 30, 2022
@ostrolucky ostrolucky changed the base branch from 2.8.x to 2.9.x January 2, 2023 21:00
@ostrolucky ostrolucky merged commit 4ff5a39 into doctrine:2.9.x Jan 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants