Skip to content

Refactor LSP to use Symfony with test coverage - #459

Open
osbre wants to merge 1 commit into
LMMS:masterfrom
osbre:feat/lsp-refactor
Open

osbre wants to merge 1 commit into
LMMS:masterfrom
osbre:feat/lsp-refactor

Conversation

@osbre

@osbre osbre commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

In order to make LSP the best it can be, we have to first make the code "maintainable". This PR refactors LSP and adds test coverage to ensure things keep working in the future. Passwords are now properly hashed using Bcrypt. Whenever someone logs-in, we re-hash the password thanks to Symfony's config.

Live environment if you would like to test it yourself: https://lmmsio-production-lj6ate.laravel.cloud/lsp (note this hosting uses ephemeral storage so files won't last long - not a code bug)

If you have a dedicated .env for production, set LSP_DATA_DIR="%kernel.project_dir%/tmp" there.

I decided to split the previously "all-in-one" route into separate paths, ie /lsp/{id}, but there's now LegacyRedirectResolver for compatibility redirects.

There's now GitHub Actions config to run tests on every change/other PRs. Feel free to ask any follow-up questions.

Don't forget to run composer i locally.

@osbre

osbre commented Jun 27, 2026

Copy link
Copy Markdown
Contributor Author

Dependency lockfile has greatly contributed to the length of this PR changes stats, so don't be too scared 👀 😅

image

@osbre

osbre commented Jun 27, 2026

Copy link
Copy Markdown
Contributor Author

One more note about passwords: the re-hashed password gets stored in the pw column, because current password column is too small for bcrypt, and since the pw column was previously unused - sounds like finally an opportunity to put it into use without having to modify the existing db schema.

@tresf

tresf commented Jun 28, 2026

Copy link
Copy Markdown
Member

One more note about passwords: the re-hashed password gets stored in the pw column, because current password column is too small for bcrypt, and since the pw column was previously unused - sounds like finally an opportunity to put it into use without having to modify the existing db schema.

My advice is to update the schema and check the hash length before comparing so we can support both old and new. I think pw once stored the temporary unencrypted version. If it's not needed, it should be removed.

@osbre
osbre force-pushed the feat/lsp-refactor branch from 0615cfb to 16cdb68 Compare June 30, 2026 19:00
@osbre

osbre commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

Thanks. Now there's 05-change-password-size.sql and 06-drop-legacy-pw-column.sql migrations attached.

I couldn't find any usage of the pw column in previous codebase, so it's safe to drop.

@osbre

osbre commented Aug 23, 2026

Copy link
Copy Markdown
Contributor Author

FYI I dropped the deployed preview environment (https://lmmsio-production-lj6ate.laravel.cloud/lsp) due to inactivity

@tresf

tresf commented Aug 23, 2026

Copy link
Copy Markdown
Member

@osbre sorry about the delay here, I stopped maintaining the website years ago so we're very short on help but you probably notice I do commit from time to time to fix critical issues with the live site.

More recently @headquarter8302 has been helping out a lot lately but perhaps between the two of use we just don't have enough experience with Symfony to be able to properly review this, which I think is why it stalled. @liushuyu was very good with these things but for some reasons out of his control he no longer has access to help currently.

I made the mistake of never even testing this.

  • Perhaps the next steps are to amend the original description for steps running it locally for those interested in testing this locally.
  • I think this PR should mention that it partially addresses the following bug report(s):
  • With regards to "dropping" the staging environment, first I'd like to thank you for setting that up. We may need it again for feedback from non-technical testers. We can request testing on our #testing channel on discord.
    * I can't imagine how frustrating it can be to receive no feedback after 2 months. I promise you this is just a side-effect of very few resources, so if this LSP is something that you plan on helping maintain, I do truly hope you can be part of the team that helps us forward through this transition.

I'll bring this up on our admin channel while I await a reply. Sorry again for the delay and hopefully we can resurrect this effort despite the project's shortcomings.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants