search

unclecheese / silverstripe-display-logic

The Display Logic module allows you to add conditions for displaying or hiding certain form fields based on client-side behavior.
BSD 3-Clause "New" or "Revised" License
74 stars 71 forks source link

Support for Silverstripe 5. Add property/return type definitions. Rename master/slave #152

Closed chrispenny closed 1 year ago

chrispenny commented 1 year ago

New major & PHP 8.1

This is going to be a new major, and now is our chance to be more declarative with property and return type requirements.

If developers have extended our base classes, then yes, there might be a lot of breaking changes for them - but it should be as simple as updating signatures. There have not been any programmatic logic changes.

Github actions

Travis is no longer free for open source. Switching to Silverstripe's Github action workflow.

Workflow (might not be visible to all) is running behat and PHPUnit tests: https://github.com/silverstripeltd/silverstripe-display-logic/actions/

Screen Shot 2023-02-15 at 3 31 03 PM

Behat

Some major changes in the new behat modules, so I bit of effort went into upgrading these.

Master / slave

I think it's reasonably well accepted that we should stop using this wording.

This would be another breaking change for developers who have extended our base classes.

Edit: And in fact, UncleCheese had already raised a PR to address this in version 2 (https://github.com/unclecheese/silverstripe-display-logic/pull/148).

Testing

My testing has been limited to forms within the CMS.

chrispenny commented 1 year ago

Just noticed that this PR was a thing: https://github.com/unclecheese/silverstripe-display-logic/pull/148

I might reconsider my names.

michalkleiner commented 1 year ago

Thanks for the work on this, @chrispenny! Will take it for a spin this week.

Re: the names, how about dispatcher/subscriber? Or dispatcher/listener? I'm not a big fan of 'primary' as it doesn't say what, and 'subordinate' from the other PR sounds too military for me :-D

michalkleiner commented 1 year ago

It would be also great to do the Travis to Github Actions change in a separate PR to be able to see the status here directly. I can possibly pick that up myself.

chrispenny commented 1 year ago

@michalkleiner sorry, I forgot to update the description. Yup, I ended up going with dispatcher and responder after seeing the work that UncleCheese had started in https://github.com/unclecheese/silverstripe-display-logic/pull/148.

I can give you access to the fork if you'd like to be able to see the actions (and can't already). https://github.com/silverstripeltd/silverstripe-display-logic/actions

chrispenny commented 1 year ago

@michalkleiner the other option is that if I close this PR, move the branch into this repo, and reopen, then the actions will display in the PR.

I'm confident that they're functioning where they are though - we've had to go through this pain a fair few times as we switch off travis.

chrispenny commented 1 year ago

Thanks for picking up all of those errors, @michalkleiner!

I've upgraded our build process, our test suite is green again, and my CMS tests are working :D

chrispenny commented 1 year ago

If there is no more feedback to come, I'll look to merge/tag this next week.