Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Refactored session rest actions to their own controller (EZP-26329) #1782

Merged
merged 2 commits into from
Sep 27, 2016

Conversation

bdunogier
Copy link
Member

@bdunogier bdunogier commented Sep 17, 2016

Implements EZP-26179
Refactoring of the changes from #1769

Delete, create and refresh session actions are deprecated in the User controller,
and moved to a new one, SessionController.

The REST functional tests now cover most of the session interactions, and passed without any issue. The code was moved as is from the updated one in the User controller, and Container calls changed to injected properties.

Copy link
Contributor

@andrerom andrerom left a comment

Choose a reason for hiding this comment

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

+1, but seems to need some adjustments to get travis and ezrobot to be green.

@bdunogier
Copy link
Member Author

Hmm indeed. I did not get these errors locally... will check.

@bdunogier bdunogier force-pushed the ezp26329-rest_session_controller_refactoring branch 3 times, most recently from 5794017 to e903165 Compare September 20, 2016 07:59
Copy link
Member

@glye glye left a comment

Choose a reason for hiding this comment

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

+1, deal with comments as you wish :)

- '@ezpublish_rest.session_authenticator'
- '%ezpublish_rest.csrf_token_intention%'
- '@?security.csrf.token_manager'
- '@?security.csrf.token_storage'
Copy link
Member

@glye glye Sep 20, 2016

Choose a reason for hiding this comment

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

I thought we standardised on double quotes for these...? (Very minor issue, it's just nice to be consistent.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Did we ? Maybe...

Gotta admit that I actually prefer single quotes because:

  • they don't require that you escape \
  • they're demanded by our coding standards for PHP (but not for YAML, I agree)

But it is indeed not consistent with what we've done so far.

Copy link
Member

Choose a reason for hiding this comment

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

Follow-up: Standardise and make it consistent, one way or the other ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to single quotes, consistency > escaping :)

use eZ\Publish\Core\REST\Common\Exceptions\NotFoundException;
use Symfony\Component\Security\Core\Exception\AccessDeniedException;
use Symfony\Component\Security\Core\Exception\AuthenticationException;
use Symfony\Component\Security\Csrf\TokenStorage\TokenStorageInterface;
Copy link
Member

Choose a reason for hiding this comment

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

Another minor nit: These are easier to read when sorted.

Copy link
Member Author

Choose a reason for hiding this comment

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

And they're sorted :)

@bdunogier
Copy link
Member Author

Did some extra cleanup, and applied @glye's comments.

Bertrand Dunogier added 2 commits September 23, 2016 08:43
delete, create and refresh session actions are deprecated in the User controller,
and moved to a new one, SessionController.
@bdunogier bdunogier force-pushed the ezp26329-rest_session_controller_refactoring branch from 6c6c1f9 to f040223 Compare September 23, 2016 06:43
@bdunogier bdunogier merged commit 0b27446 into master Sep 27, 2016
@bdunogier bdunogier deleted the ezp26329-rest_session_controller_refactoring branch September 27, 2016 12:40
# for free to join this conversation on GitHub. Already have an account? # to comment
Development

Successfully merging this pull request may close these issues.

3 participants