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

Feature/ss 5 compatibility updates #2

Merged
merged 8 commits into from
Jan 20, 2025
Merged

Conversation

jack-nzl
Copy link

  • Silverstripe 5 and php8.3 compatibility upgrades
  • Tidied up module code
  • Removed some uselesss unit tests - unit tests are still broken, but there is enough space in my brain or schedule to deal with it

@jack-nzl jack-nzl requested a review from wilr January 17, 2025 01:34
@jack-nzl jack-nzl self-assigned this Jan 17, 2025
@@ -1,60 +1,67 @@
<?php

namespace Sunnysideup\DMS\Cms;
namespace YourNamespace\Extensions;
Copy link

Choose a reason for hiding this comment

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

Should this namespace be something else? YourNamespace doesn't sound right


private static $has_one = [
private static $has_one = array(
Copy link

Choose a reason for hiding this comment

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

This is the oppostie of what we should be doing. [ is prefered over array(

/**
* Ensures that the DMS Document CMS Related and Versions fields are removed if user can't edit
*/
public function testDocumentHasNoCMSFieldsForManagingRelatedDocumentsIfCantEdit()
Copy link

Choose a reason for hiding this comment

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

These tests are worth keeping as they're testing the permission model. We're verifying that the custom permissions work (which is likely an important feature)

@wilr wilr merged commit 0d3b9fa into main Jan 20, 2025
12 of 14 checks passed
@wilr wilr deleted the feature/ss-5-compatibility-updates branch January 20, 2025 02:22
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