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

Add ZendFramework 1 support #15

Open
wants to merge 6 commits into
base: zf1
Choose a base branch
from
Open

Conversation

jean-ashton
Copy link

This is a work in progress to add ZendFramework 1 support into TESessionBundle

BagManagerConfigurationInterface::LAST_REQUEST_NAMESPACE => 'symfony/user/sfUser/lastRequest',
BagManagerConfigurationInterface::AUTH_NAMESPACE => 'symfony/user/sfUser/authenticated',
BagManagerConfigurationInterface::CREDENTIAL_NAMESPACE => 'symfony/user/sfUser/credentials',
BagManagerConfigurationInterface::CULTURE_NAMESPACE => 'symfony/user/sfUser/culture',
);

private $zf1Namespaces;

public function __construct($zf1MainNamespace, $zf1Namespaces) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add type hint: array $zf1Namespaces

Copy link
Contributor

Choose a reason for hiding this comment

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

And please add PHPDoc every where

@benja-M-1
Copy link
Contributor

IMO we should not create a new item in the configuration to add namespaces for a specific framework.
The most elegant way I can think of is to add a namespaces attribute in the configuration and inject it in the BagConfiguration used by the user.

In the end the configuration would look like (and taking into account my remark in #12:

theodo_evolution_session:
    bag_manager: theodo_evolution.session. zendframework1_bag_manager
    bag_manager_configuration: theodo_evolution.session. zendframework1_bag_manager_configuration

    namespace:
        - foo
        - bar         

And it will symplify a LOT of code in the extension too 😄

@jean-ashton
Copy link
Author

This is a great idea 👍

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.

2 participants