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.

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

Allow tag handler to be auto-wired #132

Merged
merged 6 commits into from
Jul 2, 2020
Merged

Allow tag handler to be auto-wired #132

merged 6 commits into from
Jul 2, 2020

Conversation

andrerom
Copy link
Contributor

Question Answer
Type Improvement
Target version 0.9+
BC breaks no
Doc needed yes: ibexa/documentation-developer#1071

In order to simplify use / doc on this, in a way that is future proof across versions.

TODO:

  • Implement feature / fix a bug.
  • Implement tests + specs and passing ($ composer test)
  • Fix new code according to Coding Standards ($ composer fix-cs).
  • Ask for Code Review.

@andrerom andrerom requested review from ViniTou and alongosz June 10, 2020 14:27
@andrerom andrerom mentioned this pull request Jun 10, 2020
Merged
@andrerom andrerom marked this pull request as draft June 10, 2020 15:51
alongosz
alongosz requested changes Jun 10, 2020
View reviewed changes
docs/using_tags.md Outdated Show resolved Hide resolved
docs/using_tags.md Outdated Show resolved Hide resolved
src/DependencyInjection/Compiler/DriverPass.php Outdated Show resolved Hide resolved
src/ResponseTaggerInterface.php Outdated Show resolved Hide resolved
@andrerom
Copy link
Contributor Author

andrerom commented Jun 10, 2020 •
edited
Loading

Side: I'm really not sold on Hungarian-like notation interface ...Interface, but that's a matter for different discussion, especially that I'm almost alone with this POV ;)

There are interfaces here w/o the suffix as well, so up to you :)

Edit: Dropped the interface, name conflict and not clear that we need this. Might also break if FosHttpCache 3.x adds return type as well so does not belong here.

@andrerom andrerom marked this pull request as ready for review June 10, 2020 18:37
@andrerom andrerom changed the title Allow tag handler to be auto-wired by interfaces Allow tag handler to be auto-wired Jun 10, 2020
@andrerom andrerom requested review from alongosz and vidarl June 11, 2020 10:40
@andrerom
Copy link
Contributor Author

andrerom commented Jun 25, 2020 •
edited
Loading

@ViniTou / @vidarl / @alongosz Simplified and passing, could you have a look?

Reminder this is in order to simplify HttpCache doc in ibexa/documentation-developer#1071

vidarl
vidarl approved these changes Jun 25, 2020
View reviewed changes
Copy link
Member

@vidarl vidarl left a comment

Choose a reason for hiding this comment

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

Looks good to me, except if using composer preview channel may cause other problems

@andrerom
Copy link
Contributor Author

andrerom commented Jun 25, 2020 •
edited
Loading

except if using composer preview channel may cause other problems

It might, but QA/@mnocon was ok with it for some packages where needed as canary testing for switching to it completely once it's stable.

andrerom
andrerom commented Jun 30, 2020
View reviewed changes
.travis.yml
@@ -37,7 +37,8 @@ branches:
- /^\d.\d+$/

before_script:
- travis_retry composer selfupdate
# To try to avoid running out of memory on PHP 5, and speed up install on all, using Composer 2.x here
- travis_retry composer self-update --preview
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: This will be skipped on merge up to 2.0 as @alongosz was saying there where issues with Composer 2.0 on v3, which for some reason are not on v2.5.

Copy link
Member

Choose a reason for hiding this comment

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

NOTE: This will be skipped on merge up to 2.0 as @alongosz was saying there where issues with Composer 2.0 on v3, which for some reason are not on v2.5.

If for this package there are no issues (tests are passing), then you might keep it.
We'll need to remove it anyway once composer 2 is released, because Travis already performs self-update, but stable only (AFAICS no way to configure it :( )

alongosz
alongosz requested changes Jul 1, 2020
View reviewed changes
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

Some suggestions:

.travis.yml
@@ -37,7 +37,8 @@ branches:
- /^\d.\d+$/

before_script:
- travis_retry composer selfupdate
# To try to avoid running out of memory on PHP 5, and speed up install on all, using Composer 2.x here
- travis_retry composer self-update --preview
Copy link
Member

Choose a reason for hiding this comment

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

NOTE: This will be skipped on merge up to 2.0 as @alongosz was saying there where issues with Composer 2.0 on v3, which for some reason are not on v2.5.

If for this package there are no issues (tests are passing), then you might keep it.
We'll need to remove it anyway once composer 2 is released, because Travis already performs self-update, but stable only (AFAICS no way to configure it :( )

docs/using_tags.md Outdated Show resolved Hide resolved
docs/using_tags.md Outdated Show resolved Hide resolved
@andrerom andrerom merged commit fb703c8 into 0.9 Jul 2, 2020
@andrerom andrerom deleted the autowire_tag_handler branch July 2, 2020 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vidarl vidarl vidarl approved these changes

@alongosz alongosz alongosz approved these changes

@ViniTou ViniTou Awaiting requested review from ViniTou

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@andrerom @vidarl @alongosz