godotengine / godot-asset-library

PHP frontend for Godot Engine's asset library
https://godotengine.org/asset-library
MIT License
285 stars 85 forks source link

Update GitLab repository regular expression to allow repository URLs pertaining to groups #240

Closed chucklepie closed 3 years ago

chucklepie commented 3 years ago

Hello, I have submitted a template to the asset library and put great pains into making sure everything was perfect.

I got this reply, followed by no back option and I lost everything I typed :

Error: Due to spam problems, we have to reject your asset because:
"https://gitlab.com/chucklepie-productions/tutorials/flipscreen-camera/flipscreen-template" 
doesn't look correct; it should be similar to 
"https://<gitlab instance>/<owner>/<name>". 

Please, ensure that the URL and the repository provider are correct.
Please contact the community administrators if this is not spam.

There is nothing wrong with this URL. It looks like your regex doesn't like that I have an extra folder (Gitlab calls these Groups) to put my project in?

Can this be fixed so I can upload my template, and possibly somebody put in a 'back' option so all POST data is not lost for everyone submitting, because it takes a long time to enter this information.

Thanks.

YuriSizov commented 3 years ago

I guess we don't expect a project within a series of subgroups?

chucklepie commented 3 years ago

That regex doesn't even allow for one, it expects a single project at the top level, that's just not how Gitlab works. So how do I fix it without having to mess my groups up?

btw, the lack of back option really upset me ;)

Calinou commented 3 years ago

I guess we don't expect a project within a series of subgroups?

Indeed, the repository URL needs to be the URL to a repository root, not a folder within a repository. However…

I don't think project groups were available on GitLab back when the asset library was created in early 2016, so the regex needs to be updated.

chucklepie commented 3 years ago

Thanks. So in the meantime I should move my project to the root? I'm trying to think of a way to not have to do this...

chucklepie commented 3 years ago

btw, shall I create a new request/issue for the lack of 'back'?

YuriSizov commented 3 years ago

btw, the lack of back option really upset me ;)

Sorry for that, Asset Lib is pretty outdated at the moment. The new one is in works, but it's yet to be revealed and deployed. We mostly don't fix this one as we expect the new one to address a lot of usability concerns.

Calinou commented 3 years ago

Thanks. So in the meantime I should move my project to the root? I'm trying to think of a way to not have to do this...

Yes; I don't have an ETA for fixing the regex. If you know your way around regex, you could look into it :slightly_smiling_face:

btw, shall I create a new request/issue for the lack of 'back'?

No, as the current asset library is in maintenance mode and won't get any new features. The new asset library displays validation errors on the same page, so you will keep your submitted form data in case there's an error.

chucklepie commented 3 years ago

Ok, thanks. I'll copy my text to to notepad before submitting :)

I'll see if I can have a look, yes I know regex, but I don't know the site code.

chucklepie commented 3 years ago

@Calinou

I don't know how your PR system works but I've tested this regex and it looks ok. /^https:\/\/(gitlab\.com|[^\/]+)\/[^\/]+?\/[^\/]+?(?:[^\/]+\/?)*$

The original is: ^https:\/\/(gitlab\.com|[^\/]+)\/[^\/]+?\/[^\/]+?$

This is the diff

diff --git a/src/Helpers/Utils.php b/src/Helpers/Utils.php
ndex 66b8b7f..77d39f3 100644
--- a/src/Helpers/Utils.php
+++ b/src/Helpers/Utils.php
@@ -35,10 +35,10 @@ class Utils
                 }
                 return "$repo_url/archive/$commit.zip";
             case 'GitLab':
-                if (sizeof(preg_grep('/^https:\/\/(gitlab\.com|[^\/]+)\/[^\/]+?\/[^\/]+?$/i', [$repo_url])) == 0) {
-                    $warning[] = "\"$repo_url\" doesn't look correct; it should be similar to \"https://<gitlab instance>/<owner>/<name>\". $warning_suffix";
-                } elseif (sizeof(preg_grep('/^https:\/\/(gitlab\.com)\/[^\/]+?\/[^\/]+?$/i', [$repo_url])) == 0) {
-                    $light_warning[] = "\"$repo_url\" might not be correct; it should be similar to \"https://gitlab.com/<owner>/<name>\", unless the asset is hosted on a custom instance of GitLab. $light_warning_suffix";
+                if (sizeof(preg_grep('/^https:\/\/(gitlab\.com|[^\/]+)\/[^\/]+?\/[^\/]+?(?:[^\/]+\/?)*$/i', [$repo_url])) == 0) {
+                    $warning[] = "\"$repo_url\" doesn't look correct; it should be similar to \"https://<gitlab instance>/<owner>/<optional groups>/<name>\". $warning_suffix";
+                } elseif (sizeof(preg_grep('/^https:\/\/(gitlab\.com)\/[^\/]+?\/[^\/]+?(?:[^\/]+\/?)*$/i', [$repo_url])) == 0) {
+                    $light_warning[] = "\"$repo_url\" might not be correct; it should be similar to \"https://gitlab.com/<owner>/<optional groups>/<name>\", unless the asset is hosted on a custom instance of GitLab. $light_warning_suffix";
                 }
                 return "$repo_url/-/archive/$commit.zip";
             case 'BitBucket':
chucklepie commented 3 years ago

btw, I have submitted it with new project root :)

the thumbnails look all knackered in the next screen, but I'm hoping that's just the page and not another failure on my part...

chucklepie commented 3 years ago

Thanks. I guess I can say I've half done my first godot merge request 😉

On Thu, 20 May 2021 08:22 Rémi Verschelde, @.***> wrote:

Closed #240 https://github.com/godotengine/godot-asset-library/issues/240 via #241 https://github.com/godotengine/godot-asset-library/pull/241.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/godotengine/godot-asset-library/issues/240#event-4771164173, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADCERRNASPGMZHFWBQV4CIDTOS2CVANCNFSM45FID6BQ .