Improve changes to invalidating opcache

[PhilETaylor]

Based on feedback in the CMS repo, and consultation with the WordPress project, here is a new and improved method that will handle PHP warnings better as well as making more checks before running

[nibra]

Looks reasonable to me. Any objections, @joomla-framework/maintainers ?

[PhilETaylor]

5522843 corrects the deleting of opcache before file deleted as per the references in the @see links and as per the discussion in the cms tracker

[richard67]

@PhilETaylor At least we have an idea now what's the reason: #39 , and there the link to #19 (comment) .

[PhilETaylor]

Cool, whatever. Closing.

[PhilETaylor]

Just another reason the Joomla Framework is not used by serious PHP developers on serious large scale modern PHP projects :-) A filesystem framework without any knowledge of PHP OPcache - Laughable.

[richard67]

@PhilETaylor I didn't mean I agree with him. Just that we know the reason now.

[PhilETaylor]

Ironically that comment about not adding was made by the very same person (@nibra) that added/merged the PR LMAO...

[richard67]

Maybe a Dr. Jekyll and Mr. Hyde thing ;-) (joking)

[richard67]

@nibra Your comment here #19 (comment) made me curious:

The right way to approach this is to add events to this package, so other services can react on them. But that's a bigger story that deserves its own RfC.

Is someone working on that? If not, I worry it could be beyond my skills. What would be the target version of the CMS for such a project? And what should we do in the mean time?

I think the approach here was valid for the mean time until we have a better approach.

[nibra]

I think the approach here was valid for the mean time until we have a better approach.

Agree, that' why I requested input from others. The ideal approach would be to deal with the op-code cache in a separate package and communicate using events. But as said in another place, that's a bigger story. As an interim solution this is acceprable, so re-opening.

Maybe a Dr. Jekyll and Mr. Hyde thing ;-) (joking)

Yes, sometimes I struggle between doing things right and pragmatism. Although I use to have strong opinions, I'm convincable ;)

[HLeithner]

One question would it make sense to static cache the outcome of this complex if statement, something like this

   static $opcache = null;
   if (is_null($opcache) {
	$opcache = false;
	if (ini_get('opcache.enable')
		&& function_exists('opcache_invalidate')
		&& (!ini_get('opcache.restrict_api') || stripos(realpath($_SERVER['SCRIPT_FILENAME']), ini_get('opcache.restrict_api')) === 0) {
			$opcache = true;
		}
   }
   if ($opcache && strtolower(substr($filepath, -4)) === '.php' ) {
     // invalidate
   }

IIRC ini_get and function_exists are expensive methods and deleting 1000 files could be take much longer but ymmv

[PhilETaylor]

A lot of work is happening in the CMS PR, including making this optimisation a static. I'm happy to circle back to update this PR, if there is serious interest in it actually being merged, with the lessons learned from the CMS PR.

[richard67]

One question would it make sense to static cache the outcome of this complex if statement, something like this

@HLeithner That has already been done in the J4 PR: joomla/joomla-cms#32915 .

[HLeithner]

I looked at the j4 pr and saw that all invalidation calls are at the wrong place (after the delete/rename) instead who you describe it in this pr that has to be done before deletion.

I'm of course interested in this pr because it saves a lot of boilerplate code in the cms and for people doesn't know that opcache invalidation is an important thing.

[PhilETaylor]

I looked at the j4 pr and saw that all invalidation calls are at the wrong place (after the delete/rename)

Please actually check the PR again :-) Including the 45 hidden comments, specifically this one: joomla/joomla-cms#32915 (comment) 23 hours ago which resulted in this commit joomla/joomla-cms@1c893ce

[HLeithner]

Please actually check the PR again :-) Including the 45 hidden comments, specifically this one: joomla/joomla-cms#32915 (comment) 23 hours ago which resulted in this commit joomla/joomla-cms@1c893ce

Ok I only skimmed the source and see it for the rename actions. So @nibra are you good with merging this (after static caching the ini settings)? I'm ok with it.

[richard67]

@PhilETaylor As far as I know we shall use tabs to indent PHP code.

[PhilETaylor]

phpStorm being silly with tabs/spaces not me...

[HLeithner]

Btw. You mentioned in the or some tests would be great ;)

[PhilETaylor]

well I did not break the existing tests, that's a start I guess haha

[HLeithner]

well I did not break the existing tests, that's a start I guess haha

Haha that's true ;)

[nibra]

So @nibra are you good with merging this (after static caching the ini settings)?

Yes, no objections

[richard67]

I was testing without opcache, with "normal" opcache settings and with the extreme opcache settings provided by @PhilETaylor here in the J4 CMS PR: joomla/joomla-cms#32915 (comment)

I did not really see any difference.

When testing the CMS PR, I found some issues which I will report there later.

This PR here did not help with these issues when applied in addition to that PR.

I still have to investigate if my observations are caused by mistakes in my opcache settings or me not understanding something.

So to me this PR seems not to break anything, but I also can't see any benefit.

[PhilETaylor]

Although Joomla 4 CMS includes the joomla-framework/filesystem package, the CMS doesn't actually use it (IIRC) but uses the filesystem classes from the /libraries/src/Filesystem folder

composer why joomla/filesystem shows the the only reason this framework package is loaded into the CMS on composer install, is to satisfy the dependancies in joomla/archive package

[PhilETaylor]

Sorry hit enter too quick...

So to "really" test this package's PR you either have to use joomla/filesystem in your standalone PHP application, or you need to use the Joomla Article classes somewhere in the Joomla CMS, like when extracting a Zip.

[richard67]

... or you need to use the Joomla Article classes ...

@PhilETaylor I guess you meant "Archive", not "Article", right?

[PhilETaylor]

Sorry - typo. yes, the Zip extraction for example calls File::write from the framework package and not the CMS File.php

[PhilETaylor]

I guess in a real world, we should somehow decouple the Archive and the Filesystem classes in the framework, because at the moment Joomla 4 CMS will ship with both :-( :-(

[wilsonge]

Having thought a bit about that today solving conflicts it's probably a decently high priority because the Filesystem does a totally different thing for managing the FTP/non-ftp layer. So we probably can't use the archive package in the CMS with the FTP settings.

[PhilETaylor]

I would say the commentary is outside the scope of this PR for now :)

But you are right, recoding the Archive package to not have a hard coded dependancy on the frameworkFilesystem package would allow the injection of the CMS Filesystem` package.

There are other duplications too, if you compare the folders in libraries//vendor/joomla/ and /libraries/src/ in Joomla 4, there are other duplicated folder names (so assume duplicated packages?)

With the Joomla 4 marketing tagline The power of the Joomla Framework merged into the CMS how much of the Joomla Framework are we meant to be including with composer? or is the marketing slang "merged" really mean "composer included"? The libraries folder is now over 50Mb...

[wilsonge]

With the Joomla 4 marketing tagline The power of the Joomla Framework merged into the CMS how much of the Joomla Framework are we meant to be including with composer? or is the marketing slang "merged" really mean "composer included"? The libraries folder is now over 50Mb...

What we need. Like the CMS will still need specific overrides to framework libraries (e.g. Application/Console) but it's just a move of the base classes. Generally speaking there should only be 1 of whatever - either the framework or CMS versions. The only exception that really springs to mind is HTTP - where me and michael ended up being late on the deprecation - so we've got both with the intention to deprecate the cms http package for 5.0.

Things like filesystem ideally I'd like to drop (may not be practical though). Language eventually would be nice to go from CMS to framework - but it's a large migration with the refactoring myself and Michael did so we've left that for future Joomla majors.

[PhilETaylor]

Thanks - that is what I thought. Everything is a WIP over a decade :)