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

[cache tagging] http_resp_hdr_len check #273

Merged
merged 1 commit into from
Feb 2, 2016

Conversation

wickedOne
Copy link
Contributor

fixes #269

notes:
  • as getTagsHeader and setTagsHeader are marked as depricated in the CacheInvalidator i didn't provide seperate functions for the headerLength.
  • as varnish' http_resp_hdr_len defaults to 8000 bytes, i defaulted the headerLength to 7500 bytes to be on the safe side

*/
$tagsize = max(array_map('mb_strlen', $escapedTags));
$elems = floor($this->headerLength / ($tagsize - 1)) ? : 1;

Copy link
Contributor

Choose a reason for hiding this comment

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

we could do } else { $elems = count($escapedTags); }

and then have the foreach outside the if/else, to avoid the 3 redundant lines of code in the else below.
that should not cost much extra processing power, and avoids code duplication. there would be only one chunk so the foreach would only be executed once.

Copy link
Contributor

Choose a reason for hiding this comment

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

what do you think about this idea?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, makes sense!

@dbu dbu changed the title [WIP] [cache tagging] http_resp_hdr_len check [cache tagging] http_resp_hdr_len check Jan 29, 2016
@dbu
Copy link
Contributor

dbu commented Jan 29, 2016

looks good, thanks a lot!

can you please add a note in the CHANGELOG.md file?

@wickedOne
Copy link
Contributor Author

done

@wickedOne
Copy link
Contributor Author

you think those travis errors related to this pull request?
can't seem to replicate them…

dbu added a commit that referenced this pull request Feb 2, 2016
[cache tagging] http_resp_hdr_len check
@dbu dbu merged commit 783f391 into FriendsOfSymfony:1.4 Feb 2, 2016
@dbu
Copy link
Contributor

dbu commented Feb 2, 2016

thanks! the test failure looks unrelated indeed. seems our way of starting servers has become unreliable somehow.

do you want to have a go at porting things to master? creating a branch from 1.4 and do a PR to master from that branch.

@dbu
Copy link
Contributor

dbu commented Feb 2, 2016

@wickedOne
Copy link
Contributor Author

ok, will have a go at merging 1.4 into master

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.

None yet

2 participants