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 Configuration change has no effect #21943 #21949

Merged
merged 6 commits into from Nov 17, 2018

Conversation

Projects
None yet
8 participants
@twister65
Copy link
Contributor

twister65 commented Sep 1, 2018

Pull Request for Issue #21943 .

Summary of Changes

Adds the opcache_invalidate function before loading the configuration file.
opcache_invalidate : invalidates the cached file if the modification time of the file is newer than the cached opcodes.

Testing Instructions

  1. Create a site with https://launch.joomla.org/ and change Global configuration (admin).

OR

  1. Create a site on your localhost with these server settings.
    Edit the php.ini file and set the opcache.revalidate_freq value to 60 in the opcache section :

    [opcache]
    zend_extension=php_opcache.dll
    opcache.enable=1
    opcache.enable_cli=0
    opcache.memory_consumption=128
    opcache.interned_strings_buffer=8
    opcache.max_accelerated_files=10000
    opcache.revalidate_freq=60
    opcache.fast_shutdown=1

This is my default configuration (bitnami wappstack server).

Restart Apache service.

  1. Log in as administrator
  2. System -> Global configuration
  3. Change any settings
  4. Save

Expected result

The settings in Global configuration are saved and taken into account.

Actual result

The new global configuration is not taken into account each time I save it (if patch is not applied).

@mbabker

This comment has been minimized.

Copy link
Member

mbabker commented Sep 1, 2018

This in effect cache busts the file every time the configuration is loaded (so on every request). Seems like the cache bust should happen when the global config is saved.

@zero-24

This comment has been minimized.

Copy link
Contributor

zero-24 commented Sep 1, 2018

Also here please fill out the template so we know hwy you do this and how to test and please fix the phpcs issue reported on drone:

FILE: /drone/src/github.com/joomla/joomla-cms/libraries/src/Factory.php
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 572 | ERROR | Please start your comment with a capital letter; found "//
     |       | opcode cache busting before including the filename"
--------------------------------------------------------------------------------
UPGRADE TO PHP_CODESNIFFER 2.0 TO FIX ERRORS AUTOMATICALLY
--------------------------------------------------------------------------------

As well as add a clean line before the actual include.

twister65 added some commits Sep 1, 2018

omit caching functions
I only need opcache_invalidate function to solve issue #21943
@ggppdk

This comment has been minimized.

Copy link
Contributor

ggppdk commented Sep 2, 2018

This in effect cache busts the file every time the configuration is loaded (so on every request). Seems like the cache bust should happen when the global config is saved.

Did you miss this comment ? by @mbabker
The cache clearing is called on every request

J3.9 triggers events for Global configuration save (before and after)

onApplicationBeforeSave
onApplicationAfterSave

So against the branch 3.9-dev branch

you could test / try using the code that you have already suggested here,
it should go in some always enabled plugin ... (which one ?)

	/**
	 * On Saving application configuration logging method
	 * Method is called when the application config is being saved
	 *
	 * @param   JRegistry  $config  JRegistry object with the new config
	 *
	 * @return  void
	 *
	 * @since   3.9.0
	 */
	public function onApplicationAfterSave($config)
	{
		if (function_exists('opcache_invalidate'))
		{
			opcache_invalidate(............................);
		}
	}
@SharkyKZ

This comment has been minimized.

Copy link
Contributor

SharkyKZ commented Sep 2, 2018

The code should go in com_config model, after the file has been written:

if (!JFile::write($file, $configuration))
{
throw new RuntimeException(JText::_('COM_CONFIG_ERROR_WRITE_FAILED'));
}

@twister65

This comment has been minimized.

Copy link
Contributor

twister65 commented Sep 2, 2018

When I change and save global configuration, configuration file is correctly modified (it contains the new editor), but the configuration file was not reloaded with the new configuration (cache was not updated).
opcache_invalidate : invalidates the cached file if the modification time of the file is newer than the cached opcodes.

twister65 added some commits Sep 2, 2018

@@ -458,6 +458,12 @@ private function writeConfigFile(Registry $config)
{
throw new RuntimeException(JText::_('COM_CONFIG_ERROR_WRITE_FAILED'));
}

This comment has been minimized.

@Quy

Quy Sep 2, 2018

Contributor

Remove tabs on line 461.

This comment has been minimized.

@twister65

twister65 Sep 2, 2018

Contributor

Done

@SharkyKZ

This comment has been minimized.

Copy link
Contributor

SharkyKZ commented Sep 3, 2018

I have tested this item successfully on c899ab1


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

@twister65

This comment has been minimized.

Copy link
Contributor

twister65 commented Sep 16, 2018

Please, could anyone else test this, I need this fix.
https://docs.joomla.org/Testing_Joomla!_patches/en#Recording_test_results

@SharkyKZ

This comment has been minimized.

Copy link
Contributor

SharkyKZ commented Sep 16, 2018

I suggest updating testing instructions to indicate that this can be reproduced when opcache.revalidate_freq is set to a higher value than the default.

@twister65

This comment has been minimized.

Copy link
Contributor

twister65 commented Oct 22, 2018

I suggest updating testing instructions to indicate that this can be reproduced when opcache.revalidate_freq is set to a higher value than the default.

In my php.ini file, opcache.revalidate_freq is set to 60 by default.
I have updated testing instructions, as you suggest.

@twister65 twister65 changed the title Fix Editor change does not work #21943 Fix Configuration change has no effect #21943 Nov 12, 2018

@twister65

This comment has been minimized.

Copy link
Contributor

twister65 commented Nov 12, 2018

Cloud access.net server has now the same cached behavior.
You have the same issue (configuration saving) when you launch your Joomla! site :
https://launch.joomla.org/

@anka56

This comment has been minimized.

Copy link

anka56 commented Nov 13, 2018

I have tested this item successfully on c899ab1


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

@Quy

This comment has been minimized.

Copy link
Contributor

Quy commented Nov 13, 2018

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC label Nov 13, 2018

@zero-24 zero-24 added this to the Joomla 3.9.1 milestone Nov 13, 2018

@mbabker mbabker merged commit bcf28a7 into joomla:staging Nov 17, 2018

5 checks passed

Hound No violations found. Woof!
JTracker/HumanTestResults Human Test Results: 2 Successful 0 Failed.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@joomla-cms-bot joomla-cms-bot removed the RTC label Nov 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment