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

Fix bug: dashes in theme names could break SCSS build. #4265

Merged

Conversation

demiankatz
Copy link
Member

@dltj reported that SCSS did not compile correctly when extending a theme containing a dash character. This PR fixes the problem. It's possible that an even broader regex would be more appropriate here, but I wanted to keep my fix targeted for starters.

@demiankatz demiankatz added this to the 10.1.2 milestone Feb 21, 2025
@djones-scu-edu
Copy link

Shouldn't the dash come first in the character class or be escaped?

@demiankatz
Copy link
Member Author

Thanks, @djones-scu-edu. For whatever reason, it works correctly without escaping the dash (maybe a lone unescaped dash is legal following a generic class like \w), but it definitely feels more correct to include the escaping, and adding the escape doesn't break anything. I have adjusted accordingly.

@djones-scu-edu
Copy link

I always forget in which RE variations the dash holds no special meaning in a character class if it is the first or last character in the class. In PHP, it appears to be either.

@EreMaijala
Copy link
Contributor

I think that another character we may want to add is underscore.

@demiankatz
Copy link
Member Author

demiankatz commented Feb 24, 2025

I think that another character we may want to add is underscore.

The \w metacharacter already includes the underscore (it's equivalent to [A-Za-z0-9_]), so I think we're okay here. Not sure why the decision was made to include underscore but not dash!

@demiankatz demiankatz merged commit e392d7a into vufind-org:release-10.1 Feb 24, 2025
6 checks passed
@demiankatz demiankatz deleted the release-10.1-dashes-in-themes branch February 24, 2025 12:59
@EreMaijala
Copy link
Contributor

Okay, good. I forgot that!

@demiankatz
Copy link
Member Author

Okay, good. I forgot that!

Me too; I had to look it up. But it explains why this wasn't caught sooner -- the local_theme_example has always worked and may have set a precedent that others followed instead of using dashes.

@dltj
Copy link
Contributor

dltj commented Feb 24, 2025

Yes! It was the underscores in the “local_theme_example” that had me thinking to try those. Stumbled onto the answer, in other words.

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

Successfully merging this pull request may close these issues.

4 participants