I fixed a tiny regex
Hey there! I'm Siddharth Shyniben, a 13 year old dev just coding for a hobby. I participated in the DEV Bug smash v2, and I wrote this post to tell you about it.
Open Source Background
I started coding around ~4 years back, after being inspired by my father, who does the same. Back then, my code was only on my computer, and I never dreamed of my code being viewed by everyone. Once I discovered started using GitHub around 2 years ago, all that changed. Suddenly, everyone could see my code, and report bugs, and contribute. I could host sites using GitHub pages.
GitHub was what introduced me to open-source. Seeing how beneficial open-source was to me (and everyone else), I started open sourcing almost all my work.
About This Bug
The bug was a very simple one, and most of you may not have seen it.
Bad regex for detecting SVG on Navigation Links
#14334
The dreaded error:
Code in question:
class NavigationLink < ApplicationRecord
SVG_REGEXP = /<svg .*>/i.freeze
Testing with rubular
So the problem is that the Regex expects svg text to be all on a single line:
When exporting svg from illustrator it will sometimes insert a XML schema tag first.
Yep Single line formatting fixed it:
When you use Forem to create a new community, you get to customize the sidebar, including the icons of the links. There is a function to validate the icon SVG, which looked like this:
/<svg .*>/i
But this regex wouldn't match multiline SVGs, so we had to manually remove all the newlines in the SVG.
How I Smashed It
The bug was pretty easy to smash, just had to add a single flag:
/<svg .*>/im
Navigation Link: fix regex to allow multiline SVG
#14481
What type of PR is this? (check all applicable)
- [ ] Refactor
- [ ] Feature
- [x] Bug Fix
- [ ] Optimization
- [ ] Documentation Update
Description
Allows the regex which is used to test SVG match multiline SVGs. Fixes #14334
Related Tickets & Documents
Closes #14334
QA Instructions, Screenshots, Recordings
NA
UI accessibility concerns?
NA
Added/updated tests?
- [ ] Yes
- [x] No, and this is why: Not sure if there are any tests for this
- [x] I need help with writing tests
[Forem core team only] How will this change be communicated?
Will this PR introduce a change that impacts Forem members or creators, the development process, or any of our internal teams? If so, please note how you will share this change with the people who need to know about it.
- [ ] I've updated the Developer Docs and/or Admin Guide, or Storybook (for Crayons components)
- [ ] I've updated the README or added inline documentation
- [ ] I've added an entry to
CHANGELOG.md - [ ] I will share this change in a Changelog or in a forem.dev post
- [ ] I will share this change internally with the appropriate teams
- [ ] I'm not sure how best to communicate this change and need help
- [ ] This change does not need to be communicated, and this is why not: please replace this line with details on why this change doesn't need to be shared
[optional] Are there any post deployment tasks we need to perform?
NA
[optional] What gif best describes this PR or how it makes you feel?
Reflection
This was a pretty basic issue, and I can only consider this a warmup, so there's nothing much to say here. But, I've taken up another issue, and I'll elaborate when I write about that :D.
One super important takeaway, is that once you learn one programming language, most other ones are pretty easy to use. I have zero experience in ruby, but guess what? I've smashed 2 bugs (one from the previous bug smash). So, If you're worried because you don't know a programming language used – don't worry, It might take you less than a day to learn it. This applies to a lot of stuff.
Suggestions for DEV Community Bug Smash
More bug smashes! There's a really great feeling when you contribute to OSS! Maybe you could make this a seasonal thing :D
