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

Support forward slash on branch name #3

Closed
wants to merge 2 commits into from

Conversation

vcanales
Copy link
Contributor

Using branches with forward slashes in their names causes two problems:

  1. Log files are not created, because the script tries to create the log file in a subdirectory.
  2. The script fails to checkout versions other than the initial checked out one because the initial checkout directory is not empty on subsequent tries. It also fails to create temp directories. Tested with branch name add/playground-theme-preview
[Fri Apr 19 14:10:46 2024] PHP Warning:  mkdir(): No such file or directory in /Users/vcanales/dev/github-proxy/proxy.php on line 66
[Fri Apr 19 14:10:46 2024] PHP Warning:  chdir(): No such file or directory (errno 2) in /Users/vcanales/dev/github-proxy/proxy.php on line 67
fatal: destination path 'themes' already exists and is not an empty directory.

This also creates a corrupt .zip file:

image

Vicente Canales added 2 commits April 19, 2024 14:19
Using branches with forward slashes in their names causes two problems:
1. Log files are not created, because the script tries to create the log
   file in a subdirectory.
2. The script fails to checkout versions other than the initial checked
   out one because the initial checkout directory is not empty on
subsequent tries. It also fails to create temp directories. Tested with
branch name `add/playground-theme-preview`

```
[Fri Apr 19 14:10:46 2024] PHP Warning:  mkdir(): No such file or directory in /Users/vcanales/dev/github-proxy/proxy.php on line 66
[Fri Apr 19 14:10:46 2024] PHP Warning:  chdir(): No such file or directory (errno 2) in /Users/vcanales/dev/github-proxy/proxy.php on line 67
fatal: destination path 'themes' already exists and is not an empty directory.
```
Improves readability
Comment on lines +22 to +23
$repo_name = explode('/', $repo);
$repo_name = end($repo_name);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This gets rid of this warning, which shows up on my system with php 8.3.6.

<b>Notice</b>:  Only variables should be passed by reference in <b>/Users/vcanales/dev/github-proxy/proxy.php</b> on line <b>22</b><br />

@stoph
Copy link
Owner

stoph commented Apr 21, 2024

I ended up refactoring a lot of the code for the commits and PR features. I incorporate one of your changes directly, and changed around the unique_id to be much simpler.

Thank you!

@stoph stoph closed this Apr 21, 2024
stoph added a commit that referenced this pull request Apr 23, 2024
Integrating code from #3

Co-Authored-By: Vicente Canales <[email protected]>
vcanales pushed a commit to WordPress/community-themes that referenced this pull request Apr 24, 2024
Changing where we make the call to download the theme, to make sure that
we're using stable code.

Additionally, add child theme handling.

Discussion around this:
- Automattic/themes#7719
- stoph/github-proxy#3
MaggieCabrera added a commit to WordPress/community-themes that referenced this pull request Apr 24, 2024
* Playground Previews: use correct github proxy URL

Changing where we make the call to download the theme, to make sure that
we're using stable code.

Additionally, add child theme handling.

Discussion around this:
- Automattic/themes#7719
- stoph/github-proxy#3

* change on a child theme

* pushed another test

* remove tests

* Update .github/scripts/create-preview-links.js

---------

Co-authored-by: MaggieCabrera <[email protected]>
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.

2 participants