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

Adding files and folder from anywhere (not just the custom folder) #231

Open
Llewellynvdm opened this Issue Feb 16, 2018 · 42 comments

Comments

@Llewellynvdm
Copy link
Member

Llewellynvdm commented Feb 16, 2018

I have had the need to often grab files from other places then just the custom folder in JCB, so I would like to add the feature to add any path to a file or folder and be able to include it in any component upon the compilation.

@Llewellynvdm

This comment has been minimized.

Copy link
Member Author

Llewellynvdm commented Feb 16, 2018

Basically by just extending the Component Files & Folders concept, to have a basic (the old way) and advanced (the new way) tab with the related features to include files and folders.

@Llewellynvdm Llewellynvdm moved this from In Progress to Done in Feature Requests Feb 19, 2018

@fred-the-coder

This comment has been minimized.

Copy link

fred-the-coder commented Nov 19, 2018

I am afraid this feature is not working in Windows environment.
Indeed when using an absolute path as "F:\www\joom3..." as custom files or folders, I have an error at compilation time like "'The folder path: /F:\www\joom3... does not exist, and was not added!"

Maybe I have to use a different path syntax?

Thank you

@pdward

This comment has been minimized.

Copy link

pdward commented Jan 23, 2019

Unfortunately this issue is still ongoing in a Windows environment. Currently still testing solutions - apart from changing to linux ;)

@Llewellynvdm

This comment has been minimized.

Copy link
Member Author

Llewellynvdm commented Jan 23, 2019

Okay I will open this up again ,and lets see how we can resolve this...

@Llewellynvdm Llewellynvdm reopened this Jan 23, 2019

@Llewellynvdm

This comment has been minimized.

Copy link
Member Author

Llewellynvdm commented Jan 23, 2019

@peterpetrov Just please note these few comments as we start this:

http://us2.php.net/manual/en/ref.filesystem.php#73954
https://alanhogan.com/tips/php/directory-separator-not-necessary
https://dev.to/c33s/always-use--as-directory-seperator-in-php-43l7

This is why JCB only use the '/' everywhere as I learned the PHP on both systems resolves this correctly. Yet when I see the F:\www\joom3... or the C:\wamp64\www\kindredreadersdev\media\com_kr_books_books\images\authorpen I realize the drive name is what causes the problem, but is also what we can use to detect that the user is adding a path as a windows path.

Since @pdward correctly noted on the forum that the "bug" is in the b_Structure.php file on line Line 1376 where we insure a full path as understood on Linux is used. If we detect a : in the path (we know it is a Windows path, or am I wrong) I can leave off the Unix fix and let the path remain as set by the user, will that resolve the issue.

So there is two places to update, please try these fixes:

Line 1376

if (strpos($custom['filepath'], ':') === false)
{
	$custom['file'] = '/' . trim($custom['filepath'], '/');
}
else
{
	$custom['file'] = $custom['filepath'];
}

Line 1300

if (strpos($custom['folderpath'], ':') === false)
{
	$custom['folder'] = '/' . trim($custom['folderpath'], '/');
}
else
{
	$custom['folder'] = $custom['folderpath'];
}

This will be the first step in resolving this issue, but let see... like I said before, I don't know Windows that well, so if there is a better way to detect that it is a windows path... please tell me. The idea to use this instead was also on my mind:

if (strpos($custom['folderpath'], '\') === false)
{
@Llewellynvdm

This comment has been minimized.

Copy link
Member Author

Llewellynvdm commented Jan 23, 2019

There are more places to change, and we will get to those, yet these first two are the most problematic once.

@peterpetrov

This comment has been minimized.

Copy link

peterpetrov commented Jan 23, 2019

I am going to install windows machine to be able to test all that. Also i think we should be using php to determine if it is running on windows or not. Something like that:

if (strtoupper(substr(PHP_OS, 0, 3)) === 'WIN') {
whatever_we_need();
}

@peterpetrov

This comment has been minimized.

Copy link

peterpetrov commented Jan 23, 2019

The problem i forsee here is how people are using joomla on windows so i can replicate it. I am going to install IIS + PHP and WAMP on the other hand. If anyone has different setup please share it so i can make a test environment.

@Llewellynvdm

This comment has been minimized.

Copy link
Member Author

Llewellynvdm commented Jan 23, 2019

The reason I did not go for that was since the normal \pat\to\file/name.php will still work when targeting a file in the custom folder or the Joomla root. So no need to know the environment one every case, just where the drive is also mentioned, as a full path. That is why I asked you to read the comments of those links, as they leave the idea that you only in some cases need to bend backward to catch the path variations. Tell me if I am wrong, but if we start with strtoupper(substr(PHP_OS, 0, 3)) === 'WIN' we are going to seriously have to change many aspects of the JCB structure builder. But if we only target the breaking points, the change will be minor and still work.

@fred-the-coder

This comment has been minimized.

Copy link

fred-the-coder commented Jan 23, 2019

@peterpetrov thank you for taking care....
Even redhat is now available on windows!!!
On windows, you can use EasyPhp https://www.easyphp.org/. It's coming with Apache, MySql and a PHP...

@Llewellynvdm

This comment has been minimized.

Copy link
Member Author

Llewellynvdm commented Jan 23, 2019

My thoughts are we are come to the code with strings place into the JCB GUI, and those string could be Linux based, and still run on Windows. This because we have JCB packages with those values moving between developers on various environments, so Just asking on what system are we, without actually looking at the string (path) is not ideal.

@Llewellynvdm

This comment has been minimized.

Copy link
Member Author

Llewellynvdm commented Jan 23, 2019

I for one would recommend using Joomla file paths as the one to deal with the root path, so like for example if we have a file at:

C:\wamp64\www\kindredreadersdev\media\com_kr_books_books\images\authorpen

The the correct way to add it to the GUI is:

JPATH_ROOT/media/com_kr_books_books/images/authorpen

This should then just work on Windows and Linux. This is best practice and this should already work according to my understanding.

@cpaschen can you test the above if it is true? or anyone else?

@peterpetrov

This comment has been minimized.

Copy link

peterpetrov commented Jan 23, 2019

Yep, actually i agree on that last comment. I am setting it up now and will test it as well.

@fred-the-coder

This comment has been minimized.

Copy link

fred-the-coder commented Jan 23, 2019

There should be a php PATH function, getting rid of this...It should be in Joomla actually?
Having a quick look to JPath class:

/**
	 * Searches the directory paths for a given file.
	 *
	 * @param   mixed   $paths  An path string or array of path strings to search in
	 * @param   string  $file   The file name to look for.
	 *
	 * @return  mixed   The full path and file name for the target file, or boolean false if the file is not found in any of the paths.
	 *
	 * @since   11.1
	 */
	public static function find($paths, $file)
	{
		// Force to array
		if (!is_array($paths) && !($paths instanceof Iterator))
		{
			settype($paths, 'array');
		}

		// Start looping through the path set
		foreach ($paths as $path)
		{
			// Get the path to the file
			$fullname = $path . '/' . $file;

			// Is the path based on a stream?
			if (strpos($path, '://') === false)
			{
				// Not a stream, so do a realpath() to avoid directory
				// traversal attempts on the local file system.

				// Needed for substr() later
				$path = realpath($path);
				$fullname = realpath($fullname);
			}

			/*
			 * The substr() check added to make sure that the realpath()
			 * results in a directory registered so that
			 * non-registered directories are not accessible via directory
			 * traversal attempts.
			 */
			if (file_exists($fullname) && substr($fullname, 0, strlen($path)) == $path)
			{
				return $fullname;
			}
		}

		// Could not find the file in the set of paths
		return false;
	}

Hope it helps...

@mwweb

This comment has been minimized.

Copy link

mwweb commented Jan 23, 2019

Honestly, I've tried using JCB on Windows, and although it would work 95% of the time, there were some things that just didn't function right, and it was mainly during compile.

I've tried WAMP, XAMPP, even Windows Subsystem for Linux (WSL) using Ubuntu.

I think the biggest issue that I saw was with saving of files. Linux and Windows are so very different, and regardless of whether you use WAMP, XAMPP, WSL, or other configuration, files are still saved to the Windows file system, which is where I saw the issues.

The only solution that I found that has, so far, worked "flawlessly" is Vagrant. There is something with the Vagrant system in the communication with the "box" running in VirtualBox that processes everything fine. https://www.joomlatools.com/developer/tools/vagrant/

Speed-wise, WSL was phenomenal.

@pdward

This comment has been minimized.

Copy link

pdward commented Jan 24, 2019

The following has worked for me (Windows - Bitnami wampstack-7.1.24-00) for adding custom files (not tried folders yet)

b_Structure.phpafter Line 835 $zipFullPath = str_replace('//', '/', $zipPath . '/' . $new);

if(isset($details->custom) && $details->custom == 'full') { $currentFullPath = ltrim($currentFullPath, '/'); $packageFullPath = str_replace('//', '/', $path . '/' . basename($details->newName)); }

@pdward

This comment has been minimized.

Copy link

pdward commented Jan 24, 2019

This is the output of dumping the $details array and the resulting $currentFullPath, $packageFullPath and $zipFullPath in b_Structure.php->setStatic() at line 835 for some of files already included in component compile:

stdClass Object
(
[naam] => headercheck_admin.php
[path] => c0mp0n3nt/admin/helpers
[rename] => new
[newName] => headercheck.php
[type] => file
)

$currentFullPath: C:\Bitnami\wampstack-7.1.24-0\apps\joomla\htdocs\administrator/components/com_componentbuilder/compiler/joomla_3/headercheck_admin.php
$packageFullPath: C:\Bitnami\wampstack-7.1.24-0\apps\joomla\htdocs\administrator/components/com_componentbuilder/compiler/com_SwapDoc_v0_0_18__J3/admin/helpers/headercheck.php
$zipFullPath: admin/helpers/headercheck.php

stdClass Object
(
[naam] => headercheck.php
[path] => c0mp0n3nt/site/helpers
[rename] =>
[type] => file
)

$currentFullPath: C:\Bitnami\wampstack-7.1.24-0\apps\joomla\htdocs\administrator/components/com_componentbuilder/compiler/joomla_3/headercheck.php
$packageFullPath: C:\Bitnami\wampstack-7.1.24-0\apps\joomla\htdocs\administrator/components/com_componentbuilder/compiler/com_SwapDoc_v0_0_18__J3/site/helpers/headercheck.php
$zipFullPath: site/helpers/headercheck.php

@pdward

This comment has been minimized.

Copy link

pdward commented Jan 24, 2019

The same for an Advance Adding Files (File Path: JPATH_ROOT\components\com_swapdoc\views\shift\tmpl\modal.php, Target Path: /site/views/shift/tmpl) which causes errors in File::exists($currentFullPath) and $packageFullPath0nly = str_replace(basename($packageFullPath), '', $packageFullPath); further down the function.

stdClass Object
(
[naam] => /C:\Bitnami\wampstack-7.1.24-0\apps\joomla\htdocs\components\com_swapdoc\views\shift\tmpl\modal.php
[path] => c0mp0n3nt/site/views/shift/tmpl
[rename] => new
[newName] => C:\Bitnami\wampstack-7.1.24-0\apps\joomla\htdocs\components\com_swapdoc\views\shift\tmpl\modal.php
[type] => file
[custom] => full
)

$currentFullPath: /C:\Bitnami\wampstack-7.1.24-0\apps\joomla\htdocs\components\com_swapdoc\views\shift\tmpl\modal.php
$packageFullPath: C:\Bitnami\wampstack-7.1.24-0\apps\joomla\htdocs\administrator/components/com_componentbuilder/compiler/com_SwapDoc_v0_0_18__J3/site/views/shift/tmpl/C:\Bitnami\wampstack-7.1.24-0\apps\joomla\htdocs\components\com_swapdoc\views\shift\tmpl\modal.php
$zipFullPath: site/views/shift/tmpl/C:\Bitnami\wampstack-7.1.24-0\apps\joomla\htdocs\components\com_swapdoc\views\shift\tmpl\modal.php

@pdward

This comment has been minimized.

Copy link

pdward commented Jan 24, 2019

Note this is without the fixes proposed by myself or @Llewellynvdm earlier

@Llewellynvdm

This comment has been minimized.

Copy link
Member Author

Llewellynvdm commented Jan 24, 2019

I must say I am sorry I did not realize sooner that the Windows guys are not able to see JCB in action... but I am sure we can improve this dramatically, it is not that major really.

Please keep sending more ideas and feedback on this topic.

@peterpetrov

This comment has been minimized.

Copy link

peterpetrov commented Jan 24, 2019

@Llewellynvdm

This comment has been minimized.

Copy link
Member Author

Llewellynvdm commented Jan 24, 2019

I understand completely, thanks!

@ro-ot

This comment has been minimized.

Copy link
Collaborator

ro-ot commented Jan 27, 2019

I agree with @mwweb stop trying to fix things that are not broken. Instead use the tools that fixes Windows! they are what is broken.

@Llewellynvdm

This comment has been minimized.

Copy link
Member Author

Llewellynvdm commented Jan 27, 2019

I can only smile at that... but I would like to still improve JCB to work on both. I know this will add extra load, but trust it will not be to much. We can try and move it all into one function. I will try to be as discreet as I can to avoid messing with the core flow. This cleanup will just do us all good in the long run, since little refactoring never hurt anyone.

@pjdevries

This comment has been minimized.

Copy link

pjdevries commented Mar 12, 2019

These are my thoughts:

First of all I believe Llewell thinks in the right direction by checking the filename for Windowness. If it were up to me, I would not simply check for the occurrence of a ':' though. A ':' is sort of a reserved character for uri's in general. Since we don't know what JCB has in store for us, I don't think it's wise to check for the occurence of a single character. However, we do know that a drive letter always consists of a single letter followed by a colon. We also know these are always the first two characters of the filename. So a preg_match('/^[a-z]:/i', $path) would do the trick.

Secondly I would definitely implement filename handling in a dedicated class within a general purpose library. All file and folder name manipulation within JCB should then go through standard methods to always return a normalized version of the file or folder name for a specific situation.

P.S. I am using Linux from before it was even called Linux but Unix and Windows almost from day one. Both have their pro's and cons. So does MacOS. It's a bit unfortunate that discussions about incompatibilities get contaminated with useless discussions about which OS is the best or flawed the most. Not very productive :(

Llewellynvdm added a commit that referenced this issue Mar 12, 2019

@Llewellynvdm

This comment has been minimized.

Copy link
Member Author

Llewellynvdm commented Mar 12, 2019

@pdward can you run some test, with the latest changes I made to fix this issue?

@Llewellynvdm

This comment has been minimized.

Copy link
Member Author

Llewellynvdm commented Mar 12, 2019

I don't have a MS box, so hard to see if this resolves the issue. I need some to test this, as I think we should be in the clear for the most part.

@pjdevries

This comment has been minimized.

Copy link

pjdevries commented Mar 12, 2019

I see that you do yet another translation of backward slashes into forward slashes in the new ComponentbuilderHelper::fixPath() methode. My findings are that the slashes are not the culprit but the drive letter is. You did not take that into account. Why is that?

I also found that the code around line 831 needs to be fixed in order for it to work with Windows drive letters.

@Llewellynvdm

This comment has been minimized.

Copy link
Member Author

Llewellynvdm commented Mar 12, 2019

Okay the changes I made was to remove lines that added a / in front of the drive like mentioned here:

/C:\path\to\where_ever

So that it will now look like this:

C:/path/to/where_ever

I did this to insure that scripts that target the / will behave correctly.

So my question is have you test the last changes, and does it still give issues, and can you explain more about those issues.

See I agree it is not the slashes that is the culprit, but the / being added in front of the drive path. This was removed on line 1300 and line 1378

@Llewellynvdm

This comment has been minimized.

Copy link
Member Author

Llewellynvdm commented Mar 12, 2019

I looked at line 831 and well it does not really add a / in front of the drive, or am I missing something?

$templatePath = (isset($details->custom) && $details->custom) ? (($details->custom !== 'full') ? $this->templatePathCustom . '/' : '') : $this->templatePath . '/';

It uses a global path $this->templatePath that is already correct. So it checks if it this loop is not a custom (folder) or custom (file) set of values. If it is custom set, then it checks if it is a full path. Then if it is set to a full folder or full file path we leave the $templatePath with an empty string, which is the correct action.

Because all full paths should already have the drive set, by either using the Joomla! path place holders, like JPATH_BASE (which is the recommended) or by using the full path with the drive as part of the path.

@pdward

This comment has been minimized.

Copy link

pdward commented Mar 13, 2019

On windows line 833 is effectively prepending a '/' to the $item (which is correct) as $templatePath is empty.

If I remove the prepended '/' then the custom files are found.

However there is a new bug due to the compile/componentfolder not existing
e.g. /administrator/components/com_componentbuilder/compiler/com_SwapDoc_v0_0_22__J3/

Haven't checked with custom folders

@pjdevries

This comment has been minimized.

Copy link

pjdevries commented Mar 13, 2019

I did this to insure that scripts that target the / will behave correctly.
I am not sure how to read this. Whether you mean any slashes in de designated path or whether you mean the single / at the beginning of a path, making it an absolute path.

If it is the former, I believe translation of \ into / is not necessary. PHP handles them correctly on Windows. Even when you mix the two within one and the same filename. However, I do agree it is good practice to standardize on always using /.

If it is the latter, I think you must also take Windows drive letters into account. Before you pushed the latest changes regarding this matter, this is what I did on my Windows development system:

I changed lines 830-834

// set the template folder path
$templatePath = (isset($details->custom) && $details->custom) ? (($details->custom !== 'full') ? $this->templatePathCustom . '/' : '') : $this->templatePath . '/';
// set the final paths
$currentFullPath = str_replace('//', '/', $templatePath . '/' . $item);

into

// set the template folder path
$templatePath = (isset($details->custom) && $details->custom) ? (($details->custom !== 'full') ? $this->templatePathCustom . '/' : '') : $this->templatePath . '/';
if (preg_match('/^[a-z]:/i', $item) === false)
{
    $templatePath .= '/';
}
// set the final paths
$currentFullPath = str_replace('//', '/', $templatePath . $item);

line 1300

$custom['folder'] = '/' . trim($custom['folderpath'], '/');

into

$custom['folder'] = preg_match('/^[a-z]:/i', $custom['folderpath']) === false
    ? '/' . trim($custom['folderpath'], '/')
    : trim($custom['folderpath'], '/');

and line 1376

$custom['file'] = '/' . trim($custom['filepath'], '/');

into

$custom['file'] = preg_match('/^[a-z]:/i', $custom['filepath']) === false
    ? '/' . trim($custom['filepath'], '/')
    : trim($custom['filepath'], '/');

Not my preferred way of coding because there is a lot of duplicate code. But it's merely a proof of concept. Better would be to move the drive check to a library function.

It also does not incorporate your fixPath() method (yet), but it solves the Windows problem I encountered with Component Files & Folders Advanced (using full system paths). I'm quite confident it will also solve the Windows problem in other places with similar logic.

@pjdevries

This comment has been minimized.

Copy link

pjdevries commented Mar 13, 2019

I rolled back my drive letter fix to test your latest changes. It brings back the errors I had before, showing paths with drive letters and a leading /. Adding the fix for lines 830-834 I mentioned above, fixes the problem with the Windows drive letter for this specific instance.

I also get the compiler warnings @peterpetrov mentions about files not found and not being copied. Undoing the ComponentbuilderHelper::fixPath() method, solves that.

@Llewellynvdm : My suggestion is to rollback that latest change, since it does not seem to solve anything and even introduces new issues.

@peterpetrov : Apparently you are testing another situation than I am. Can you let me know what so I can try to reproduce and fix that?

@Llewellynvdm

This comment has been minimized.

Copy link
Member Author

Llewellynvdm commented Mar 13, 2019

Okay I am trying to fix something I can't see.

I have rolled back the changes, I am going to try again.

@pjdevries

This comment has been minimized.

Copy link

pjdevries commented Mar 13, 2019

Let me try. I know a thing or two about PHP and Joomla! and have a Windows system. Just tell me where the problem area's are, apart from the ones I already found.

Llewellynvdm added a commit that referenced this issue Mar 13, 2019

@Llewellynvdm

This comment has been minimized.

Copy link
Member Author

Llewellynvdm commented Mar 13, 2019

Okay take two... I have adapted the fix to include your proposed fix. Please test and let me know.

Llewellynvdm added a commit that referenced this issue Mar 13, 2019

Llewellynvdm added a commit that referenced this issue Mar 13, 2019

@pdward

This comment has been minimized.

Copy link

pdward commented Mar 13, 2019

Still not quite working.

The PHP_OS method worked for me
if(strtoupper(substr(PHP_OS, 0, 3)) === "WIN") { $currentFullPath = ltrim($currentFullPath, '/'); }


I'm using BITNAMI WAMP Stack 7.1.24 on Windows 10

In my component I'm adding Advanced (full path) Custom Files and folders:
File Path: JPATH_ROOT/components/com_swapdoc/views/shift/tmpl/modal.php
Target Path: /site/views/shift/tmpl

Folder Path: JPATH_ROOT/images/sampledata/fruitshop
Target Path: /site/images

With the @Llewellynvdm latests changes (ignoring my hack) I get error on $currentFullPath:
/C:\Bitnami\wampstack-7.1.24-0\apps\joomla\htdocs/components/com_swapdoc/views/shift/tmpl/modal.php

Note - Adding Basic custom files worked for me previously but haven't tried this now

@pdward

This comment has been minimized.

Copy link

pdward commented Mar 13, 2019

I think the problem is because I'm using JPATH_ROOT instead of C:
Hence the regex ('/^[a-z]:/i') on line 1304 isn't applicable

@pdward

This comment has been minimized.

Copy link

pdward commented Mar 13, 2019

Updating the dynamic path before checking for a drive/windows full path works

@Llewellynvdm

This comment has been minimized.

Copy link
Member Author

Llewellynvdm commented Mar 13, 2019

Okay let me also change that, should not be a big deal.

Llewellynvdm added a commit that referenced this issue Mar 13, 2019

@Llewellynvdm

This comment has been minimized.

Copy link
Member Author

Llewellynvdm commented Mar 14, 2019

@pdward please confirm that this issue is resolved for you?

@pdward

This comment has been minimized.

Copy link

pdward commented Mar 14, 2019

Yes - the latest staging version works for me for advanced files / folders on windows

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