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

NEW Update RelatedPages feature for ManyManyThrough #93

Conversation

NightJar
Copy link
Contributor

@NightJar NightJar commented Jun 1, 2018

Which also provides the ability to version the relationship.

relies on symbiote/silverstripe-gridfieldextensions#260

Copy link
Contributor

@robbieaverill robbieaverill left a comment

Choose a reason for hiding this comment

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

It's worth noting that this same change has been labelled as an API breakage in core, so we might need to be careful about how we release it. Context: silverstripe/recipe-cms#12. Also the issue is labelled as a patch change, but targets master (next minor release) and has an enhancement prefix on the commit - which should it be (keep in mind previous related issue)

'SortOrder' => DBInt::class
];

// For backwards compatibility these must match a traditional 'many_many' definition.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use /** */ comment syntax for PHPDocs

@NightJar NightJar changed the title NEW Update RelatedPages feature for ManyManyThrough WIP: NEW Update RelatedPages feature for ManyManyThrough Jun 10, 2018
@NightJar NightJar changed the base branch from master to 2.1 June 25, 2018 23:20
@NightJar NightJar force-pushed the pulls/2.0/through-related-pages branch from 57f1fdd to 68fbede Compare June 26, 2018 03:44
@NightJar
Copy link
Contributor Author

OK, so I've refactored a little bit to preserve the existing many_many relationship that will still return the expected ManyManyList, while maintaining the many many through under a new name, and using that to preform the ordering. This way both are available, for a very minimal overhead in technical debt (50 consecutive characters).

Since both relationships use the same backing data, hopefully this will see Backwards Compatibility truly maintained.

@NightJar NightJar changed the base branch from 2.1 to 2.0 June 26, 2018 03:54
@NightJar NightJar changed the title WIP: NEW Update RelatedPages feature for ManyManyThrough NEW Update RelatedPages feature for ManyManyThrough Jun 26, 2018
@robbieaverill
Copy link
Contributor

robbieaverill commented Jul 1, 2018

This PR requires symbiote/silverstripe-gridfieldextensions#260 right? If so, should we target it at 2.2 as an enhancement?

@NightJar
Copy link
Contributor Author

NightJar commented Jul 1, 2018

In regards to this module, I'd regard it as a bugfix. It requires a fix from a dependency, but I think the constraint already allows for it.

Lemme check.


The merge for gridfieldextensions was to the master branch, and was a new feature, so will be tagged as 3.2.0.

So will also need to bump the constraint here.
BRB; updating PR. (and cherrypicking dependency back to the 3 branch of gridfieldextensions)

Copy link
Contributor

@robbieaverill robbieaverill left a comment

Choose a reason for hiding this comment

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

Merge on green

@dhensby
Copy link
Contributor

dhensby commented Jul 4, 2018

Composer dependencies don't resolve on travis :(

@NightJar
Copy link
Contributor Author

NightJar commented Jul 4, 2018

I'm aware; should hopefully be a simple matter to resolve - it's more finding the time for a context switch that's preventing this happening quickly.

Copy link
Contributor

@robbieaverill robbieaverill left a comment

Choose a reason for hiding this comment

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

Merge once CWP 2.1.0 is released, in case it gets merged up and released before then

@NightJar
Copy link
Contributor Author

NightJar commented Jul 5, 2018

Thank you for sorting the constraints @robbieaverill ❤️

@robbieaverill robbieaverill changed the base branch from 2.0 to 2.2 July 26, 2018 22:43
Dylan Wagstaff and others added 4 commits July 27, 2018 10:44
Which also provides the ability to version the relationship.
With the recent update to use ManyManyThroughList instead of the old
ManyManyList for the 'RelatedPages' feature on BasePage, this
has altered the return type of the applicable getters, and the two types
of many many relationship do unfortuantely not implement the same
interface. This breaks backwards compatiblity, so this commit in turn
attempts to preserve it through keeping both types of relationship.

Using the through relationship to maintain the ability to order ones
preferences on the related pages list, while keeping the default
'RelatedPages' relationship to return the ManyManyList where anyone
might be expecting it.

In the background they use the same relationship information, most
applicably the same join table that actually defines the relationships
through the new SilverStripe 4 feature to be able to custom name a
model's database table. This has worked in our advantage by naming the
same table as is generated in the old style many_many relationship.
The alterations to support ordering many many through lists in
gridfieldextensions will be released as 3.2.0 - the current requirements
of cwp/cwp (this module) require ^3.1 - which will pull in the version
we need, however it's better to update the minimum version since the
update to features here rely upon it. It is improbable yet plausible
that someone could end up with an inability to sort their many_many
releated pages releationship, which is what parent commits of this one
are attempting to prevent.
@robbieaverill robbieaverill force-pushed the pulls/2.0/through-related-pages branch from 3429979 to bd2d911 Compare July 26, 2018 22:45
Copy link
Contributor

@robbieaverill robbieaverill left a comment

Choose a reason for hiding this comment

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

Rebased, merge on Travis green (into CWP 2.2 branch since this is marked as an enhancement)

@robbieaverill robbieaverill merged commit 1f55a0e into silverstripe:2.2 Jul 26, 2018
@robbieaverill robbieaverill deleted the pulls/2.0/through-related-pages branch July 26, 2018 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants