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

replacing apc with apcu functions. #1951

Merged
merged 1 commit into from
Jun 6, 2022

Conversation

f1-outsourcing
Copy link
Contributor

Description (*)

changes for apcu

Related Pull Requests

Fixed Issues (if relevant)

  1. Fixes OpenMage/magento-lts#<issue_number>

Manual testing scenarios (*)

Just enabling the apc backend in the config like this, should give you some caching already.

 28 <config>
 29     <global>
 30         <!--
 31         -->
 32         <cache>
 33             <backend>apc</backend>
 34             <prefix>uo_</prefix>
 35             <auto_refresh_fast_cache>0</auto_refresh_fast_cache>
 36         </cache>

apcu

Questions or comments

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)
  • Add yourself to contributors list

@github-actions github-actions bot added Component: Core Relates to Mage_Core Component: lib/Zend Component: lib/* Relates to lib/* labels Jan 2, 2022
@addison74
Copy link
Contributor

Thank you for your contribution.

Before merging this PR all OpenMage files must be analyzed after apc word. Let's not wake up to unpleasant surprises. I found in a file a reference to apc_fetch and the PR omits this file.

I remain of the opinion that the APC/APCu should be removed from the OM code. There are solutions like CM_Redis that offer much more flexibility. If the server is stored on NVMe then I no longer see the purpose of the cache in memory (in the case of a single store).

@f1-outsourcing
Copy link
Contributor Author

Thank you for your contribution.

Before merging this PR all OpenMage files must be analyzed after apc word. Let's not wake up to unpleasant surprises. I found in a file a reference to apc_fetch and the PR omits this file.

Yes, please have close look at it. I also do not understand what the Zend full page cache option does in the admin interface, since I seem to always get items in apc cache regardless if that option is enabled or disabled.

I remain of the opinion that the APC/APCu should be removed from the OM code. There are solutions like CM_Redis that offer much more flexibility. If the server is stored on NVMe then I no longer see the purpose of the cache in memory (in the case of a single store).

Imho arguments referring to third party solutions or own use cases are not really worth mentioning. Afaik is the most sold drive type a hdd and not nvme.
For me the comparison between using 'magento-lts' and 'magento-lts with apc enabled' only counts, and whether or not there are (technical) issues related to using/activating apcu like eg with security, future support etc.

Copy link
Contributor

@luigifab luigifab left a comment

Choose a reason for hiding this comment

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

zf1-future did the same changes.

@f1-outsourcing
Copy link
Contributor Author

WTF is with this????

@kiatng
Copy link
Contributor

kiatng commented Feb 16, 2022

@f1-outsourcing As an active member of this community, I'm unable to review this PR because, 3 reasons: I'm not familiar with APC. I'm not using it in any of my projects. It has no impact for me. Other active members probably have similar reasons.

I have 7 open PRs and others have even more. What I do is this: all my PRs were implemented in my projects, either already in production or going to be. As a developer, I did lots of changes to the core, of these changes, I select those that would benefit the community and make PRs of them. I do not need my PRs to get merged in OM in order to use them, they were already merged in either my production or development branches. (I'm no git expert, but somehow, I don't get merged conflicts when OM updates were applied.)

I hope this explains why nothing has happenned to this PR. I also show with my own example, how you can benefit from your own contribution first, before the adoption, if at all, from the community.

@f1-outsourcing
Copy link
Contributor Author

@f1-outsourcing As an active member of this community, I'm unable to review this PR because, 3 reasons: I'm not familiar with APC. I'm not using it in any of my projects. It has no impact for me. Other active members probably have similar reasons.

I have 7 open PRs and others have even more. What I do is this: all my PRs were implemented in my projects, either already in production or going to be. As a developer, I did lots of changes to the core, of these changes, I select those that would benefit the community and make PRs of them. I do not need my PRs to get merged in OM in order to use them, they were already merged in either my production or development branches. (I'm no git expert, but somehow, I don't get merged conflicts when OM updates were applied.)

I hope this explains why nothing has happenned to this PR. I also show with my own example, how you can benefit from your own contribution first, before the adoption, if at all, from the community.

I am happy to give you access to my 'test environment'. So you do not need to configure/install anything, everything is up and running as we speak.

@kiatng
Copy link
Contributor

kiatng commented Feb 16, 2022

I am happy to give you access to my 'test environment'. So you do not need to configure/install anything, everything is up and running as we speak.

Thanks for the offer. However, I am a very slow learner, it would take me at least a couple of weeks to learn APC. I have to decline.

@luigifab
Copy link
Contributor

luigifab commented Feb 16, 2022

We can also says...
This is not a Magento/OpenMage bug, This is a Zend bug, There is a fix with ZF1-Future, So we are adding the same patch.

@f1-outsourcing
Copy link
Contributor Author

I am happy to give you access to my 'test environment'. So you do not need to configure/install anything, everything is up and running as we speak.

Thanks for the offer. However, I am a very slow learner, it would take me at least a couple of weeks to learn APC. I have to decline.

:D I can appreciate such answer. Much better than the headless chickens that seem to be working on m2

@Sekiphp
Copy link
Contributor

Sekiphp commented Feb 19, 2022

https://php.uz/manual/en/intro.apc.php

This extension is considered unmaintained and dead. However, the source code for this extension is still available within PECL GIT here: https://git.php.net/?p=pecl/caching/apc.git.

We can backport changes from Zend1 future, but there should be more problematic code parts, for example, there are getting some values from PHP config via ini_get, but this config keys are removed in APCU.

For example:

  • apc.rfc1867
  • apc.rfc1867_name
  • apc.rfc1867_prefix

If we let APC/APCU support in OM, we should fix all calls. This is ideal case for writing unit tests :-)

@f1-outsourcing
Copy link
Contributor Author

f1-outsourcing commented Feb 21, 2022

Hi @Sekiphp, thanks for looking at this.

Sorry this remark is lacking insufficient knowledge of Zend and apc. I am not sure what the what/where these are being used, looks to me they are specific to uploading[1]. I do not even get why uploading is being addressed. (If this is only for an upload progress bar, I would definitely ignore this in apc[2])

I would just try to keep this fix as simple as possible. For users the biggest advantage is in the caching of 'download' requests. Maybe a fix could include just removing the upload related stuff?

The changes I applied shows the apcu cache like this, with the prefix from the configuration file local.xml. I am not even using this store yet, and there are already quite a lot of hits.
apcu

[1]
https://php.uz/manual/en/apc.configuration.php#ini.apc.rfc1867
https://php.uz/manual/en/apc.configuration.php#ini.apc.rfc1867-prefix

[2]
https://www.php.net/manual/en/session.upload-progress.php

Copy link
Contributor

@luigifab luigifab left a comment

Choose a reason for hiding this comment

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

Same as zf1.

@fballiano fballiano merged commit a978a51 into OpenMage:1.9.4.x Jun 6, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jun 6, 2022

Unit Test Results

1 files  ±0  1 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
7 runs  ±0  5 ✔️ ±0  2 💤 ±0  0 ❌ ±0 

Results for commit a978a51. ± Comparison against base commit f99c9b0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Core Relates to Mage_Core Component: lib/* Relates to lib/*
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants