Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[FrameworkBundle] HttpCache is not longer abstract#26356

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
nicolas-grekas merged 1 commit intosymfony:2.7fromlyrixx:http-cache-abstract
Mar 16, 2018

Conversation

@lyrixx
Copy link
Member

QA
Branch?master
Bug fix?no
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets
LicenseMIT
Doc PR

I don't really know why this class was abstract in the first place.
But it's not needed. A fresh symfony 4 application can use this class
directly without extending it.

@stof
Copy link
Member

stof commentedMar 1, 2018

this was never necessary AFAICT. It looks like a mistake by trying make it parallel to AppKernel extending the Symfony class, while there was actually nothing to implement in AppCache from day one.

@stof
Copy link
Member

stof commentedMar 1, 2018

However, this class has a typehinting issue in the constructor. It typehints HttpKernelInterface, but the implementation actually expects a KernelInterface (which has much more methods)

@lyrixx
Copy link
MemberAuthor

So is it a bug fix ?
Should I also fix the type-hint ?

Simperfit reacted with thumbs up emoji

@nicolas-grekas
Copy link
Member

lets' make it a bug fix and fix the type hint?

@lyrixx
Copy link
MemberAuthor

OK, I will do that

@lyrixxlyrixx changed the base branch frommaster to2.7March 14, 2018 17:49
@lyrixx
Copy link
MemberAuthor

done

/**
* @paramHttpKernelInterface $kernel AnHttpKernelInterface instance
* @param string$cacheDir The cache directory (default used if null)
* @paramKernelInterface $kernel AnKernelInterface instance
Copy link
Contributor

Choose a reason for hiding this comment

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

- An KernelInterface instance+ A KernelInterface instance

lyrixx reacted with thumbs up emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Fixed

@javiereguiluz
Copy link
Member

I'm confused: is this a bug fix or a new feature? The branch is2.7 but the milestone is4.1.. Also, just asking: won't this need any change at all in end-users code in any case? If yes, we'd need a note in the UPGRADE file.

@lyrixx
Copy link
MemberAuthor

lyrixx commentedMar 16, 2018•
edited
Loading

@javiereguiluz I updated the PR to be a "bug fix" as requested. Indeed the milestone should be changes.

And this change does not require any change in end-users code.

javiereguiluz reacted with thumbs up emoji

@lyrixxlyrixx modified the milestones:4.1,2.7Mar 16, 2018
@nicolas-grekas
Copy link
Member

Thank you@lyrixx.

@nicolas-grekasnicolas-grekas merged commit4d075da intosymfony:2.7Mar 16, 2018
nicolas-grekas added a commit that referenced this pull requestMar 16, 2018
This PR was merged into the 2.7 branch.Discussion----------[FrameworkBundle] HttpCache is not longer abstract| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets || License       | MIT| Doc PR        |---I don't really know why this class was abstract in the first place.But it's not needed. A fresh symfony 4 application can use this classdirectly without extending it.Commits-------4d075da [FrameworkBundle] HttpCache is not longer abstract
@lyrixxlyrixx deleted the http-cache-abstract branchMarch 16, 2018 13:06
@fabpot
Copy link
Member

How is this a bug? This is definitely a "new feature". It does not fix anything.

@lyrixx
Copy link
MemberAuthor

@fabpot initially the PR targeted master. But Nikos asked me to put it as a bug fix.

  • The typehint was not the right one. So to me it's a bug fix as it did not work if you pass aHttpKernelInterface instance
  • The removal of the abstract keyword might not be a bugfix, But I thinks it does not harm

WDYT? Should I revert the PR and re-open a new one against master ?
I could also just revert the removal of abstract

@fabpot
Copy link
Member

"But I thinks it does not harm" is exactly what I don't like. If we start doing that, most PRs do not harm (at least, that's the initial reasoning), but something it does. So, we should follow our policy strictly: only bug fixes on non-master branches.

In this case, I would revert the abstract change in all branches but master (you can do that directly in the code without PRs as it would mean 2 extra PRs).

@lyrixx
Copy link
MemberAuthor

lyrixx commentedMar 21, 2018•
edited
Loading

Ok I got it. Sorry about that.

I have added6073066 to the 2.7 branch. I wanted to merge 2.7 in 2.8 but there was conflicts about other changes. I'm not really able to solve theme :/
I will re revert the commit in master when it will be merged.

Thanks

This was referencedApr 2, 2018
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@javiereguiluzjaviereguiluzjaviereguiluz approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@chalasrchalasrchalasr approved these changes

+1 more reviewer

@ogizanagiogizanagiogizanagi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

2.7

Development

Successfully merging this pull request may close these issues.

8 participants

@lyrixx@stof@nicolas-grekas@javiereguiluz@fabpot@ogizanagi@chalasr@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp