Skip to content

Conversation

@brianteeman
Copy link
Contributor

continuing the work to namespace joomla4 one admin component at a time.

please wait for drone to successfully complete before reviewing.

@zero-24
Copy link
Contributor

zero-24 commented Jul 11, 2018

please wait for drone to successfully complete before reviewing.

Why? Given that drone fails on something that is not related to this changes here anyway?

Copy link
Contributor

@zero-24 zero-24 left a comment

Choose a reason for hiding this comment

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

I have just added some comments. The general changes look good on a quick review.


/*
* Check the PHP Version. It is already checked in JUpdate.
* Check the PHP Version. It is already checked in Update.
Copy link
Contributor

Choose a reason for hiding this comment

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

This result into a useless comment can we convert this please to

Check the PHP Version. It is already checked in the Update prozess

define('_JEXEC', 1);
}

use Joomla\CMS\Language\Text;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is 3rd Party Code. I'm not sure if we really should touch this.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we do we should also fix JFile to File but for me I'm not sure if that overrides apply anyway. (given that we now use namespaced versions of that code)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is not 3rd party code.

Copy link
Contributor

Choose a reason for hiding this comment

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

My point about the overrides etc still stands even when there is a Joomla header in that file.

Copy link
Contributor

Choose a reason for hiding this comment

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

This file is our hook point into the restore.php file. It is not third party.


HTMLHelper::_('behavior.keepalive');

$twofactormethods = \Joomla\CMS\Helper\AuthenticationHelper::getTwoFactorMethods();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should't this be imported by use also?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no idea

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it can be


HTMLHelper::_('behavior.keepalive');

$twofactormethods = \Joomla\CMS\Helper\AuthenticationHelper::getTwoFactorMethods();
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. Should be imported by use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should it>? you dont seem sure

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes please import it using a user and than use the namespaced version

@zero-24
Copy link
Contributor

zero-24 commented Jul 11, 2018

Please add to the following test cases:

Test case 1

  • Install this branch
  • update to a newer version of this com_joomlaupdate

Test case 2

  • install a older branch
  • update with a package that includes this changes.

use Joomla\CMS\Router\Route;
use Joomla\CMS\Factory;
use Joomla\CMS\HTML\HTMLHelper;
use Joomla\CMS\Helper\\AuthenticationHelper;
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove extra \

use Joomla\CMS\User\UserHelper;
use Joomla\CMS\Client\ClientHelper;
use Joomla\CMS\Client\FtpClient;
use Joomla\CMS\Fileystem\File;
Copy link
Contributor

Choose a reason for hiding this comment

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

Spelling Fileystem

use Joomla\CMS\Response\JsonResponse;
use Joomla\CMS\Session\Session;
use Joomla\CMS\Client\ClientHelper;
use Joomla\CMS\Fileystem\File;
Copy link
Contributor

Choose a reason for hiding this comment

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

Spelling Fileystem

@wilsonge wilsonge merged commit 48003c5 into joomla:4.0-dev Jul 16, 2018
@wilsonge wilsonge added this to the Joomla 4.0 milestone Jul 16, 2018
@wilsonge
Copy link
Contributor

Thanks!

@brianteeman
Copy link
Contributor Author

thanks

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants