Skip to content

Conversation

@come-nc
Copy link
Contributor

@come-nc come-nc commented Dec 2, 2021

Signed-off-by: Côme Chilliet come.chilliet@nextcloud.com

nextcloud/3rdparty#934

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc come-nc self-assigned this Dec 2, 2021
@come-nc
Copy link
Contributor Author

come-nc commented Dec 2, 2021

Error: The method ScssPhp\ScssPhp\Compiler::toHSL is internal to ScssPhp but called from OCA\Theming\Util (see https://psalm.dev/175)

Can someone familiar enough look into https://scssphp.github.io/scssphp/docs/changelog.html and see how big the impact of this bump is?

We had 1.4.0 before so it’s a pretty big bump, I’m not sure if the psalm spotted problem is the only one.

This ports away from using now internal functions from scssphp.

Signed-off-by: Carl Schwan <carl@carlschwan.eu>
@CarlSchwan
Copy link
Member

I just did some small grepping and the only misused was the one reported by psaml and I fixed it. Other than that locally everything seems to still works and I followed https://scssphp.github.io/scssphp/docs/#security-considerations to do a bit of hardening

@CarlSchwan CarlSchwan force-pushed the fix/bump-scssphp/scssphp-to-1.8.1 branch 3 times, most recently from f9f53e6 to b706bba Compare December 2, 2021 11:43
@CarlSchwan CarlSchwan force-pushed the fix/bump-scssphp/scssphp-to-1.8.1 branch from 30098c8 to dfb569f Compare December 2, 2021 14:58
Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

We can merge but note that this scsscacher should go away as soon as we have the time to :)

@skjnldsv skjnldsv merged commit b067ae7 into master Dec 3, 2021
@skjnldsv skjnldsv deleted the fix/bump-scssphp/scssphp-to-1.8.1 branch December 3, 2021 10:55
@come-nc
Copy link
Contributor Author

come-nc commented Dec 6, 2021

@skjnldsv You merged here but it was needed to merge nextcloud/3rdparty#934 first and then rebase here to use the merge commit.

@ChristophWurst
Copy link
Member

@come-nc make your PR a draft next time 😉

@come-nc
Copy link
Contributor Author

come-nc commented Dec 6, 2021

@come-nc make your PR a draft next time wink

But can it get approvals while still being a draft? I wait for CI and approvals on server side before merging 3rdparty side, I thought this was not possible with a draft.

@come-nc
Copy link
Contributor Author

come-nc commented Dec 6, 2021

(3rdparty was merged, fixing PR server-side incoming)

@nickvergessen
Copy link
Member

But can it get approvals while still being a draft?

yes

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.

5 participants