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

Disallowing periods in slugs (Response to issue#2383) #2385

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

alinaghale88
Copy link
Contributor

I have updated the regular expression pattern in isSlug.js to disallow periods for strings of type slug. I have also ensured that the modification made does not impact all the correct conditions the existing pattern supports at present.

test-results

Checklist

  • PR contains only changes related; no stray files, etc.
  • README updated (where applicable)
  • Tests written (where applicable)
  • References provided in PR (where applicable)

@WikiRik
Copy link
Member

WikiRik commented Mar 31, 2024

Just fyi;

This is the basis of the validator: https://gist.github.com/hagemann/382adfc57adbd5af078dc93feef01fe1

In my opinion any changes to isSlug should still conform to that gist.

I'll check later if that's the case. In the meantime; don't forget to add additional tests.

Copy link

codecov bot commented Apr 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (4197b86) to head (5e19e96).
Report is 17 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##            master     #2385   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          107       107           
  Lines         2449      2449           
  Branches       619       619           
=========================================
  Hits          2449      2449           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@alinaghale88
Copy link
Contributor Author

@WikiRik I have added a new invalid test case and made further changes to ensure that the regular expression complies with all the valid and invalid test cases. Please review and provide your feedback.

@@ -1,6 +1,7 @@
import assertString from './util/assertString';

let charsetRegex = /^[^\s-_](?!.*?[-_]{2,})[a-z0-9-\\][^\s]*[^-_\s]$/;
let charsetRegex = /^(?![-_])^[^\s](?!.*?[-_]{2,})[a-z0-9*&\\]*(?:[-_][a-z0-9*&\\]+)*[^-_\s](?!\.)$/;

Choose a reason for hiding this comment

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

Should 'a-a' be considered a slug? I ran this example with this regex and it is not highlighting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the string 'a-a' should be considered a slug @Zhuochengyu

Thank you for pointing out this exception. I have pushed another update for the regex and also included a new test case.

@alinaghale88
Copy link
Contributor Author

Just fyi;

This is the basis of the validator: https://gist.github.com/hagemann/382adfc57adbd5af078dc93feef01fe1

In my opinion any changes to isSlug should still conform to that gist.

I'll check later if that's the case. In the meantime; don't forget to add additional tests.

Just fyi;

This is the basis of the validator: https://gist.github.com/hagemann/382adfc57adbd5af078dc93feef01fe1

In my opinion any changes to isSlug should still conform to that gist.

I'll check later if that's the case. In the meantime; don't forget to add additional tests.

@WikiRik The reference you provided seems to be changing any string that cannot be considered a slug to an actual definition of a slug. But the isSlug function's role is to validate whether or not a provided string is of type slug.

@jfaMan
Copy link

jfaMan commented Apr 17, 2024

@alinaghale88 As mentioned here, the original regex effectively required at least 3 characters, which your revision doesn't seem to do when I tested it. Not sure if it's technically required, but just something I thought I'd mention. I use the isLength validator for my use case anyway to catch those. This change otherwise fixes my issue with special characters passing.

@jfaMan
Copy link

jfaMan commented Apr 26, 2024

Just fyi;
This is the basis of the validator: https://gist.github.com/hagemann/382adfc57adbd5af078dc93feef01fe1
In my opinion any changes to isSlug should still conform to that gist.
I'll check later if that's the case. In the meantime; don't forget to add additional tests.

Just fyi;
This is the basis of the validator: https://gist.github.com/hagemann/382adfc57adbd5af078dc93feef01fe1
In my opinion any changes to isSlug should still conform to that gist.
I'll check later if that's the case. In the meantime; don't forget to add additional tests.

@WikiRik The reference you provided seems to be changing any string that cannot be considered a slug to an actual definition of a slug. But the isSlug function's role is to validate whether or not a provided string is of type slug.

I believe he means in the sense that any characters Slugify retains when it 'slugifies' a string are considered valid characters in a slug, and isSlug uses this as the basis of how it validates slugs.

@@ -1,6 +1,6 @@
import assertString from './util/assertString';

let charsetRegex = /^[^\s-_](?!.*?[-_]{2,})[a-z0-9-\\][^\s]*[^-_\s]$/;
let charsetRegex = /^(?!.*(?:-|_){2,})(?![-_])(?!.*(?:-|_)$)[a-z0-9*&\\]*(?:[-_][a-z0-9*&\\]+)*$/;
Copy link

Choose a reason for hiding this comment

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

Something else I noticed, but is there a reason this allows &, * and \ special characters? The original allows \ though I'm not sure why since Slugify ignores \, and @WikiRik mentioned that Slugify is the basis of the regex for isSlug. Slugify also ignores * and replaces & with 'and', so I would consider all of them to be invalid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants