wp2static icon indicating copy to clipboard operation
wp2static copied to clipboard

Bedrock support: Use home URLs instead of site URL or paths

Open oriolarcas opened this issue 4 years ago • 13 comments

WP2Static assumes that the base path of some directories is the root of the Wordpress install, but in some environments like Bedrock these directories can be in a different subdirectory (like "wp/"). This assumption adds private directories to the detected URLs. This PR removes that assumption.

oriolarcas avatar Nov 20 '21 12:11 oriolarcas

Hi @oriolarcas, many thanks for this PR!

I'm excited to get it merged, but also want to make sure we're not changing behaviour in an unexpected way for non-Bedrock/custom dir sites.

There are some failing tests in this PR - could you please run composer run-script test locally? You should see some errors like this:

1) WP2Static\FileHelperTest::testGetListOfLocalFilesByDir
TypeError: WP2Static\FilesHelper::getListOfLocalFilesByDir(): Argument #2 ($base_url) must be of type string, array given, called in /home/runner/work/wp2static/wp2static/tests/unit/FileHelperTest.php on line 149

/home/runner/work/wp2static/wp2static/src/FilesHelper.php:47
/home/runner/work/wp2static/wp2static/tests/unit/FileHelperTest.php:149
/home/runner/work/wp2static/wp2static/vendor/10up/wp_mock/php/WP_Mock/Tools/TestCase.php:307

@palmiak I think you're running Bedrock - what do you think about this PR?

leonstafford avatar Nov 20 '21 23:11 leonstafford

I did some work to try adding Bedrock to our integration tests. In order to get crawling working, I had to change https://github.com/leonstafford/wp2static/blob/1db23dc1ed9e0c13bc82d8efa5f2e1831dd463ec/src/Crawler.php#L116 to $absolute_uri = new URL( rtrim( SiteInfo::getURL( 'home' ), '/' ) . $root_relative_path );

Did you also need to make a change in that area, or is there some other way to deal with that?

john-shaffer avatar Nov 21 '21 05:11 john-shaffer

I made some other changes to get crawling and post-processing working, and added Bedrock to the integration tests.

I believe that this is the correct behavior in general and not a Bedrock-specific fix. However, this is a breaking change, and it's possible that someone has a working website that depends on the current behavior. We may want to add an option like "Use WP home URL as the site root".

john-shaffer avatar Nov 28 '21 00:11 john-shaffer

@john-shaffer cool - time for a BC release version, seems all the fun in PHP these days :D

Still some failing tests on the PR though

leonstafford avatar Nov 28 '21 08:11 leonstafford

(or does it need GH actions re-run/forced run after last push?)

leonstafford avatar Nov 28 '21 08:11 leonstafford

Sorry, I should have mentioned that this is still WIP

john-shaffer avatar Nov 30 '21 23:11 john-shaffer

I have some work on adding an option for this at https://github.com/john-shaffer/wp2static/commit/f18c15c967df84feaf5dd430dae71143aa0bc10d

However, I find it impossible to predict what the impact of these changes will be on real sites.

john-shaffer avatar Feb 18 '22 02:02 john-shaffer

@leonstafford can you take a look at my review notes from earlier? If we can address those, I would be okay with merging this. I'm pretty sure that using the home URL is the correct behavior, and I guess that adding an option for the old incorrect behavior wasn't the best idea. We can do without that.

john-shaffer avatar Feb 18 '22 03:02 john-shaffer

Sorry for the delay, @john-shaffer, will get to reviewing your notes this soon!

leonstafford avatar Mar 02 '22 02:03 leonstafford

@john-shaffer sorry for delays. If you could please resolve conflicts and choose whether you want the option from your branch in, I'll merge this in soon. We can see what kind of reports if any we get in from the repo users before bundling into next major release

leonstafford avatar Mar 24 '22 06:03 leonstafford

Hate to be "that guy", but any update on this?
Seems like most everything here is mostly ready to go, just waiting on a reply from @john-shaffer?

SharkWipf avatar Jul 18 '22 12:07 SharkWipf

Hi @SharkWipf thanks for keeping pressure on, I've got a lot of catching up to do on this repo!

leonstafford avatar Jul 20 '22 08:07 leonstafford

Sorry for not answering @john-shaffer, thank you for taking over the PR.

oriolarcas avatar Aug 25 '22 12:08 oriolarcas