Page MenuHomePhabricator

Resolved task T159405 and T159458
Needs ReviewPublic

Authored by Kamsuri5 on Mar 11 2017, 9:39 AM.

Details

Reviewers
Yaron_Koren
Patch without arc
git checkout -b D596 && curl -L https://phabricator.wikimedia.org/D596?download=true | git apply

Diff Detail

Repository
rEPFM extension-PageForms
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 1696
Build 2734: arc lint + arc unit

Event Timeline

Kamsuri5 created this revision.Mar 11 2017, 9:39 AM
Yaron_Koren edited edge metadata.Mar 12 2017, 1:25 AM

Thanks for the patch! Did you test these? If not, please do.

Kamsuri5 updated this revision to Diff 1569.EditedMar 12 2017, 7:22 AM
  • Updated previous revision

I am sorry for the previous patch, it was not correct.

Great. Did you test these? And what's the deal with the .arcconfig file?

Kamsuri5 added a comment.EditedMar 12 2017, 4:48 PM

I have checked this includes/PF_AutoeditAPI.php but was not able to check libs/PageForms.js.
.arcconfig file is for Arcanist, to check whether it is in project's root directory or not.

Kamsuri5 updated this revision to Diff 1570.Mar 12 2017, 4:51 PM
  • Updated previous revision

Alright. Let me know when you've tested the whole thing - testing the code is part of the task!

I have tested the second patch separately but not able to get how to check it in the application, so it would be really helpful if you can guide me a bit on that.

Okay, doing just unit testing is fine. However, for the URL validation, it would be good to also have the old validation, at least for some of the protocols. Could you have it do the big regex validation for URLs that start with http, https, ftp or ftps? (I know the current regex has a different list of protocols, but that one is kind of strange.)

^(ht|f)tps?:\/\/[a-z0-9-\.]+\.[a-z]{2,4}\/?([^\s<>\#%"\,\{\}\\|\\\^\[\]`]+)?$
This regex expression works fine for URL validation. Should i submit a patch having URL validation using this regex and only for http, https, ftp and fttps ?

Well, the validation should also check that it starts with one of the allowed protocols. And doing something like "(ht|f)tps?" is clever, but maybe "too clever": it would be hard to add additional protocols to that set. By the way, this doesn't have to be one regex line - however you can get it working is good, as long as it's easily modifiable.

Kamsuri5 updated this revision to Diff 1580.EditedMar 16 2017, 3:05 PM
  • Updated regex expression

You are right that trick won't be a feasible solution. I have changed the code with regex expression having option to add additional protocols. Please have a look.

It still needs to check against wgUrlProtocols somewhere...

Kamsuri5 added a comment.EditedMar 16 2017, 3:13 PM

You mean to say that we should check via both ways that is via regex as well as wgUrlProtocols ?
Okay, this way we will be able to check for common URLs as we are not sure whether wgURLProtocols is working correct.

Yes - it needs to check that the URL uses one of the allowed protocols, and if the protocol is "http", etc., it needs to check whether the formatting is right.

This comment was removed by Kamsuri5.

You could do it that way, yes.

Kamsuri5 updated this revision to Diff 1582.Mar 16 2017, 3:57 PM
  • Updated to check for both formatting and protocols

This seems like it will reject any URL that starts with, say, "mailto:" - no?

Kamsuri5 updated this revision to Diff 1583.Mar 16 2017, 8:01 PM
  • Updated regex to check "mailto" as well
Kamsuri5 added a comment.EditedMar 17 2017, 4:13 AM

Is it fine, or you wanted something else because what I got from your earlier comment was that we will use regex for checking formatting and wgUrlProtocols for checking protocols?
And sir I submitted the autoedit.php patch earlier, you haven't merged that? Was there any problem in that?

Kamsuri5 updated this revision to Diff 1584.Mar 17 2017, 4:20 AM
  • Updated code

@Kamsuri5 - I thought it over, and I regret to inform you that I won't take you for this GSoC project. There are five students vying for this project, so my standards have to be very high, and unfortunately you made too many errors during the creation of this patch. (Though the PF_Autoedit.php part is correct.) I'm trying to narrow it down as quickly as possible so that the students who aren't selected will have a chance to find another project this summer - I hope you look for something else and can find another project, whether with Wikimedia or another organization. Good luck.

Alright tha's your call but actually the errors in the second patch occurred because I was not able to get from your comment that what exactly you are saying. I have contributed a lot of time to this project in past few days to have an in-depth knowledge of the same. This is my first time I am contributing to open source so maybe i was not able to understand a few things. But even after making errors I was the one who stood there to correct then. It is not that I lack in knowledge of coding. I can only request you to reconsider my work and would like to thank you for all your help and time.

I'm sorry - I know the process is not very fair, but I'm sticking with my decision, and I hope you can find another project.