Skip to content
This repository

Implementing new file system #1007

Closed
wants to merge 19 commits into from

4 participants

Christophe Demko AmyStephen elinw ianmacl
Christophe Demko

This pull request implement new file system for Joomla!

<?php
$filesystem = JFilesystem::getInstance();
$file = $filesystem->getFile('/path/to/file');
$file->contents = 'My contents'; // This will store 'My contents' into the file
$file->open('r'); // This will open the file for reading
$string = $file->readLine(); // This will read a line
$file->close(); // This will close the file

// Iterating on files
foreach ($file->iterateLine() as $line)
{
    // Deal with $line
}

$file->delete(); // This will destroy the file

See the docs for other features

Christophe Demko chdemko closed this
Christophe Demko

Reworking unit test for testing remote file system

Christophe Demko chdemko reopened this
AmyStephen

This is beautiful. Once we learn how to use JFileSystemElement - that should be very powerful.

Excellent stand alone package - will get a lot of us in this world.

Any reason not to simply bring in Jxmlelement? Appears to be the only dependency.

Will be interested in seeing how JRegistry is adapted for this class. Seems like it could be simplified. It's another great piece of code.

Christophe Demko

@AmyStephen
"This is beautiful. Once we learn how to use JFileSystemElement - that should be very powerful."

Look at the manual (not already complete but will be done next days)

"Any reason not to simply bring in Jxmlelement? Appears to be the only dependency. "

I don't understand what you are meaning

AmyStephen

I most definitely will be studying the documentation, appreciate you doing that.

In this package, there is only one external dependency:

+jimport('joomla.utilities.xmlelement');

Wondering if the single method needed could be included to remove even that dependency? It's a great stand-alone package and something that will be needed time and time again by developers. Having no external dependencies at all will make it that much easier to use - good, good work. Appreciate it very much.

elinw
elinw commented

I'm confused are you losing all of history?

Christophe Demko

I have rebased the code to avoid a lot of commit in the platform history

ianmacl
Owner

I don't think we want to add the static method in JFactory.

libraries/joomla/filesystem/element.php
((608 lines not shown))
  608
+	 *
  609
+	 * @link    http://php.net/manual/en/function.readlink.php
  610
+	 *
  611
+	 * @since   12.2
  612
+	 */
  613
+	protected function getLink()
  614
+	{
  615
+		return readlink($this->_fullpath);
  616
+	}
  617
+
  618
+	/**
  619
+	 * Sets the element group
  620
+	 *
  621
+	 * @param   int  $group  The new group ID
  622
+	 *
  623
+	 * @return  int|false  Returns the group ID of the element, or FALSE if an error occurs.
1
elinw
elinw added a note

Would you make this mixed please. Also please spell out integer and boolean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
libraries/joomla/filesystem/element/directory.php
((8 lines not shown))
  8
+ */
  9
+
  10
+defined('JPATH_PLATFORM') or die;
  11
+
  12
+/**
  13
+ * A Directory handling class
  14
+ *
  15
+ * @property-read  RecursiveIteratorIterator  $files        Iterator on files.
  16
+ * @property-read  RecursiveIteratorIterator  $directories  Iterator on directories.
  17
+ *
  18
+ * @method                            bool create(int $permissions)                   create the directory
  19
+ * @method                            bool delete()                                   delete the directory
  20
+ * @method                       int|FALSE copy(JFilesystemElementDirectory $dest)    copy the directory
  21
+ * @method                       int|FALSE copyFromFile(JFilesystemElementFile $src)  copy from a file
  22
+ * @method       RecursiveIteratorIterator files(array $options)                      iterate on files
  23
+ * @method       RecursiveIteratorIterator directories(array $options)                iterate on directories
2
elinw
elinw added a note

Looks like you have tabs here.

Christophe Demko
chdemko added a note

No tabs in my code

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

Somewhere deep inside this something is calling /tests/suites/unit/joomla/table/JTableCategoryTest.php" which is now in the legacy package.

Christophe Demko

I'm not calling /tests/suites/unit/joomla/table/JTableCategoryTest.php"

elinw
elinw commented

Oh cool the tests ran again and it's not failing any more so whatever you were calling that was calling JTableCategory probably isn't any more.

ianmacl ianmacl commented on the diff
libraries/joomla/filesystem/element/file.php
((65 lines not shown))
  65
+
  66
+	/**
  67
+	 * @var  string  The file opening mode
  68
+	 */
  69
+	private $_mode;
  70
+
  71
+	/**
  72
+	 * Constructor
  73
+	 *
  74
+	 * @param   string       $path       Element path.
  75
+	 * @param   JFilesystem  $system     Element file system
  76
+	 * @param   string       $signature  Signature
  77
+	 *
  78
+	 * @since   12.2
  79
+	 */
  80
+	protected function __construct($path, JFilesystem $system, $signature)
1
ianmacl Owner
ianmacl added a note

This should be public.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ianmacl ianmacl commented on the diff
libraries/joomla/filesystem/filesystem.php
((43 lines not shown))
  43
+	 * @since   12.2
  44
+	 */
  45
+	private $_context;
  46
+
  47
+	/**
  48
+	 * Constructor
  49
+	 *
  50
+	 * @param   string  $prefix   The file system prefix.
  51
+	 * @param   array   $options  The stream context options.
  52
+	 * @param   array   $params   The stream context params.
  53
+	 *
  54
+	 * @link    http://fr.php.net/manual/en/function.stream-context-create.php
  55
+	 *
  56
+	 * @since   12.2
  57
+	 */
  58
+	protected function __construct($prefix,  $options,  $params)
1
ianmacl Owner
ianmacl added a note

This should be public.

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

It took me quite a while to actually understand how this package was structured. There is an incredibly liberal use of magic getters, setters, callers and static methods and calls that get sent all over.

I think there are some basic elements of the package that are useful and good. However, I think that the approach taken is over complicated.

There is a lot of effort put into enabling accessing files using different formats using $fileElement->readIni and proxying to static accessors. I think a better approach would be to create classes that would take the JFilesystemElement or JFilesystem objects as parameters in their constructors and implement that functionality that way.

The other concern I have is some of the non-obviousness of the interface in general. In my opinion, the way that FileElement->location works is rather unusual. Using the value TRUE to move the cursor to the end seems odd. Perhaps using actual numbers to set the cursor would be better and it could then be operator on using normal arithmetic.

It wasn't entirely clear to me what the differences were between read and push and write and pull. For some reason I just am not sure what you are doing there.

ianmacl
Owner

So I think in general we want to rework the filesystem package but I personally don't think this is it. I'm going to close this until Christophe or somebody can pick this up again and engage in more discussion on it.

ianmacl ianmacl closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.