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

EZP-20426 - Legacy cronjobs and scripts should be launchable from Symfony CLI #558

Merged
merged 1 commit into from Feb 18, 2013

Conversation

@lolautruche
Copy link
Contributor

commented Feb 14, 2013

Work in progress, DO NOT MERGE

Ref https://jira.ez.no/browse/EZP-20426.
Related PR : ezsystems/ezpublish-kernel#229

Tasks

  • Refactoring of eZScript allowing to update settings while recalling the singleton
  • Avoid ezpAutoloader class to be declared twice
@andrerom

View changes

autoload.php Outdated
*/
class ezpAutoloader
// Check if ezpAutoloader exists because it can be already declared if running in the Symfony context (e.g. CLI scripts)
if ( !class_exists( 'ezpAutoloader' ) )

This comment has been minimized.

Copy link
@andrerom

andrerom Feb 15, 2013

Member

class_exists( 'ezpAutoloader', false ) ?

And maybe return early approach to avoid the indentation change, so

if ( class_exists( 'ezpAutoloader', false ) )
{
    return;// we assume eZBase autoload has been setup as well.
}

This could even be in the start of the file right?

This comment has been minimized.

Copy link
@lolautruche

lolautruche Feb 15, 2013

Author Contributor

No, doesn't work for some reason. And I didn't want to take risks.
However, 👍 for class_exists( 'ezpAutoloader', false ) 😃

This comment has been minimized.

Copy link
@andrerom

andrerom Feb 15, 2013

Member

ok, might be needed for APC to understand it.

@lolautruche

View changes

lib/ezsession/classes/ezsession.php Outdated
@@ -505,7 +505,7 @@ static public function regenerate( $updateBackendData = true )
*/
static public function remove()
{
if ( !self::$hasStarted )
if ( !self::$hasStarted || session_id() === '' )

This comment has been minimized.

Copy link
@lolautruche

lolautruche Feb 15, 2013

Author Contributor

Had to add session_id() check because, for some reason, CLI scripts can use sessions and in that case session is forced to start. However, Symfony does not handle sessions in CLI (which seems to be normal), so session is never started.
This avoids a warning when doing the session_destroy() below.

This comment has been minimized.

Copy link
@andrerom

andrerom Feb 16, 2013

Member

If so, either check if current sapi is CLI, or add comment in the code instead of here ;)

This comment has been minimized.

Copy link
@lolautruche

lolautruche Feb 18, 2013

Author Contributor

You're right @andrerom 😃
I'll add a small comment.

Jérôme Vieilledent
Implemented EZP-20426 - Legacy cronjobs and scripts should be launcha…
…ble from Symfony CLI

Refactoring of eZScript allowing to update settings while recalling the singleton.
{
$GLOBALS['eZScriptInstance'] = new eZScript( $settings );
}
else if ( !empty( $settings ) )
{
$GLOBALS['eZScriptInstance']->updateSettings( $settings );

This comment has been minimized.

Copy link
@andrerom

andrerom Feb 18, 2013

Member

This is broken already before, but not really better with this change:
If your asking for a instance with different settings, then your asking for a different instance.

This comment has been minimized.

Copy link
@lolautruche

lolautruche Feb 18, 2013

Author Contributor

I know and this should not happen. This is mostly to keep BC as much as possible (it happens in the case of CLI scripts in Symfony context only)

This comment has been minimized.

Copy link
@andrerom

andrerom Feb 18, 2013

Member

It would have been a bit cleaner if CLIHandler dealt with this, either by creating and setting the instance, or making updateSettings() public.

@andrerom

This comment has been minimized.

Copy link
Member

commented Feb 18, 2013

+1, but instance change could have been more de coupled.

@lolautruche

This comment has been minimized.

Copy link
Contributor Author

commented Feb 18, 2013

@andrerom I'll try to refactor this a bit then

@lolautruche

This comment has been minimized.

Copy link
Contributor Author

commented Feb 18, 2013

@andrerom This is actually not possible since the singleton call which would update the settings is controlled by the scripts themselves. Hence the CLILoader cannot have control here...

andrerom added a commit that referenced this pull request Feb 18, 2013
Merge pull request #558 from ezsystems/legacy_scripts
EZP-20426 - Legacy cronjobs and scripts should be launchable from Symfony CLI

@andrerom andrerom merged commit cc75474 into master Feb 18, 2013

1 check passed

default The Travis build passed
Details

@andrerom andrerom deleted the legacy_scripts branch Feb 18, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
You can’t perform that action at this time.