? Pending

User tests: Successful: Unsuccessful:

avatar laoneo
laoneo
21 Apr 2022

Pull Request for comment #37592 (comment).

Summary of Changes

Removes Replaces the CMS/Filesystem dependencies in the classmap generator.

Testing Instructions

  • Remove the file /administrator/cache/autoload_psr4.php
  • Open the back end

Actual result BEFORE applying this Pull Request

The removed file is regenerated and the site loads as expected.

Expected result AFTER applying this Pull Request

The removed file is regenerated and the site loads as expected.

avatar laoneo laoneo - open - 21 Apr 2022
avatar laoneo laoneo - change - 21 Apr 2022
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 21 Apr 2022
Category Libraries
avatar laoneo
laoneo - comment - 21 Apr 2022

@Fedik here you have a filesystem pr.

avatar laoneo laoneo - change - 21 Apr 2022
Labels Added: ?
avatar Quy
Quy - comment - 21 Apr 2022

I have tested this item successfully on 7d8ecc3


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

avatar Quy Quy - test_item - 21 Apr 2022 - Tested successfully
avatar Shubhamverma2796
Shubhamverma2796 - comment - 21 Apr 2022

I have tested this item successfully on 7d8ecc3


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

avatar Shubhamverma2796 Shubhamverma2796 - test_item - 21 Apr 2022 - Tested successfully
avatar richard67 richard67 - change - 21 Apr 2022
Status Pending Ready to Commit
avatar richard67
richard67 - comment - 21 Apr 2022

RTC


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

avatar HLeithner
HLeithner - comment - 21 Apr 2022

Please don't blindly replace File::write with file_put_contents thanks. There are multiple reasons to keep it as it is.

avatar richard67
richard67 - comment - 21 Apr 2022

@HLeithner It seems there are different opinions because @wilsonge approved it. Am setting the RLDQ label to flag it for further discussion/decision.

avatar laoneo
laoneo - comment - 22 Apr 2022

@HLeithner please elaborate why this should not work. It was not a blind replacement. I'v even tested it when the cache folder doesn't exist, it loads the map into the ram thanks to #33263. The site crashes afterwards because the cachecontroller can't find the folder. So we are pretty safe here to move to native PHP.

avatar laoneo laoneo - change - 22 Apr 2022
Labels Added: ? ?
avatar HLeithner
HLeithner - comment - 22 Apr 2022

FIle::write does much more then just write a file to disk. it creates the folder (ok should exists) but most important it invalidates the opcache.

Never the less on the roadmap on j5 we have the point to use asynchronous functions where it make sense and where it's possible. Don't know yet if it's useful at this point but at least should be considered.

avatar brianteeman
brianteeman - comment - 22 Apr 2022

this is a great example of why low level changes need much better test instructions and descriptions

avatar richard67 richard67 - change - 22 Apr 2022
Status Ready to Commit Pending
avatar richard67
richard67 - comment - 22 Apr 2022

Back to pending due to reasons stated above. I should have remembered the opcache thing when setting RTC.


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

avatar laoneo
laoneo - comment - 22 Apr 2022

this is a great example of why low level changes need much better test instructions and descriptions

And this is a great example how to not participate in an open source project as it doesn't bring any value and pisses of contributors. Instead of propose a suggestion what needs to be changed

but most important it invalidates the opcache

My opcache settings are as follows, so what do I need to change to make the pr fail when opcache kicks in as it worked on my end?
image

avatar richard67
richard67 - comment - 22 Apr 2022

My opcache settings are as follows, so what do I need to change to make the pr fail when opcache kicks in as it worked on my end?

@laoneo You could try with the settings mentioned in the description of PR #36878 :

Use a server with OPcache enabled and the settings opcache.validate_timestamps=1 and opcache.revalidate_freq=300

Your screenshot doesn't show the values for these settings.

avatar laoneo laoneo - change - 22 Apr 2022
Labels Added: ?
Removed: ?
avatar laoneo
laoneo - comment - 22 Apr 2022

Now I can reproduce the opcache issue. Reverted to use the framework package write function, so we don't need the full CSM stack.

So I guess we can do another round of testing as it does the same as before, just using the framework package.

avatar richard67
richard67 - comment - 22 Apr 2022

Now the only remaining change is the alpha ordering of the use statements. I think this can be tested by review.

@laoneo Could you update the description of the PR accordingly? Thanks in advance.

Silly me ... was confused by the almost same use statements. But I see it changes from CMS to framework package.

avatar laoneo laoneo - change - 22 Apr 2022
The description was changed
avatar laoneo laoneo - edited - 22 Apr 2022
avatar laoneo laoneo - change - 28 Apr 2022
Labels Removed: ? ?
avatar HLeithner
HLeithner - comment - 1 May 2022

The framework throws exceptions which was a return false before. this have to be caught. Both File::write and Folder:folders need to be in a try catch block now.

avatar laoneo
laoneo - comment - 2 May 2022

Catching now the exceptions.

avatar HLeithner HLeithner - change - 4 May 2022
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2022-05-04 12:23:35
Closed_By HLeithner
avatar HLeithner HLeithner - close - 4 May 2022
avatar HLeithner HLeithner - merge - 4 May 2022
avatar HLeithner
HLeithner - comment - 4 May 2022

thanks

Add a Comment

Login with GitHub to post a comment