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

Fix APC for when internal key name is entry_name #10600

Merged
merged 3 commits into from
Jul 16, 2016

Conversation

photodude
Copy link
Contributor

@photodude photodude commented May 23, 2016

Pull Request for Part of Issue #10220

Summary of Changes

Some APC(u) modules changed the internal key name from key to entry_name. HHVM is one such case where the APC module uses a different naming.

This addresses the 3 known types of keys for APC(u)

Testing Instructions

Merge by code review.

Passing test with var_dump of the entries in our unit tests showing HHVM apc key name is entry_name https://travis-ci.org/photodude/joomla-cms/jobs/132170969

// Some APC modules changed the internal key name from key to entry_name, HHMV is one such case
}
elseif (isset($key['entry_name']))
{
// Some APC modules changed the internal key name from key to entry_name, HHMV is one such case
Copy link
Contributor

Choose a reason for hiding this comment

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

HHVM, not HHMV

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow, I was clearly up too late. My eyes were seeing what they expected to see, rather than what was on screen. Thanks for the catch on that typo.

@mbabker
Copy link
Contributor

mbabker commented May 23, 2016

APCu probably needs to be checked too.

@photodude
Copy link
Contributor Author

@mbabker We could patch it in APCu also, although I'm not aware of any APCu modules using entry_name for the internal key name. Currently HHVM doesn't have an APCu module since they preinstall the ACP module.

@mbabker
Copy link
Contributor

mbabker commented May 23, 2016

Seems odd they would use an outdated/abandoned PHP extension but ¯\_(ツ)_/¯

@photodude
Copy link
Contributor Author

About as odd as why they installed memcache and memcached in the default settings.

Some APCu modules may have changed the internal key name from `key` or `info` to `entry_name`
@photodude
Copy link
Contributor Author

@mbabker I've added the fix to APCu as well on the off chance that HHVM might use APCu someday.

@photodude
Copy link
Contributor Author

Is there any interest in moving forward with this PR?

@mbabker
Copy link
Contributor

mbabker commented Jun 11, 2016

I'd say it should be merged. If it's gonna sit and wait for people who have PHP environments with APC(u) to come along and test it's going to go nowhere.

@photodude
Copy link
Contributor Author

@wilsonge Can this be merged now?

@wilsonge
Copy link
Contributor

wilsonge commented Jul 3, 2016

RTC on Review. For 3.6.1


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10600.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jul 3, 2016
@wilsonge wilsonge added this to the Joomla 3.6.1 milestone Jul 3, 2016
@photodude
Copy link
Contributor Author

Thanks @wilsonge

@roland-d roland-d merged commit 7756c18 into joomla:staging Jul 16, 2016
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jul 16, 2016
@photodude photodude deleted the patch-6 branch July 16, 2016 17:31
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.

5 participants