VirtualPage getCMSFields problems with LiteralFields #1008

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants

Ho0m3s commented May 4, 2014

Fixed the Main Virtual Page to allow for not displaying LiteralFields twice when using DataExtension See bugs fixes #2827, #2850. This is using the same techniques as $member->getCMSFields();

Ho0m3s added some commits May 4, 2014

@Ho0m3s Ho0m3s Modifed the VirtualPage getCMSFields to fix the issue #2827 which wa…
…s happening when creating Literal Fields on Virtual Pages . I used the same technique as the one use by $member->getCMSFields() i use the beforeUpdateCMSFields method
94e0cf0
@Ho0m3s Ho0m3s Modifed the VirtualPage getCMSFields to fix the issue #2827 which wa…
…s happening when creating Literal Fields on Virtual Pages . I used the same technique as the one use by $member->getCMSFields() i use the beforeUpdateCMSFields method
cd2ec13
Contributor

tractorcow commented May 4, 2014

Thanks @Ho0m3s , but not sure about the phpdoc fixes. Why does __call no longer have a @return? The hasField is probably ok, since there's no point in repeating phpdoc on an overloaded method.

You'll need to re-squash your commits and show a test case that demonstrates the fix. Thanks!

Ho0m3s commented May 4, 2014

Thanks @tractorcow , I am new to GitHub so i probably messed up.
I don't understand your comments. I did not do anything to phpdoc. The fix I am proposing is related to Virtual Page class.

If you try to overwrite the "VirtualPageMessage" LiteralField in a DataExtension you end with 2 Literal Field the original and the one the custom one. There is no way with the orginal VirutalPage class to remove that original LiteralField, because it gets called twice. By applying what you guys came up for bug#2827, to the VirtualPage i was able to customize the Literal Field and remove the original.

The Fix i applied is the one you guys implemented in the $member->getCMSField() with this->beforeUpdateCMS()... I just applied that to the VirtualPage class.

I committed twice cause i left some debug stuff in the first. The meat of the fix is found on the first commit. This is where you will see the changes from the main branch. Is that what you mean by re-squash my commits?

And also what do you mean by test case. It pretty easy to replicate.

Once again i am new at GitHub i looked at your documentation for participating and it seemed pretty open. I am missing something? If so, please let me know.

Contributor

tractorcow commented May 5, 2014

No problem, you're off to a good start. I just noticed some strange diffs
in your changelog.

I suggest you look at a few practices that will help you get off the ground:

  • Rebase your commits against the latest branch regularly, this will keep
    your PR up to date.
  • Squash all commits; Typically a PR should only have one commit against it
    to ensure we are making atomic changes only.
  • Test cases should be standard for just about every PR. Check out
    http://doc.silverstripe.com/framework/en/topics/testing/ for how to write a
    test. Typically a test demonstrates a failure that would occur without a
    change to the code (i.e. you'd test that a page with one label field,
    assigned a virtual page, only generates one label field). It's not enough
    to simply describe an error situation without an automatable test case. If
    you noticed, we have a service called travis which runs all test cases for
    each pull request, so we can see the output of this test case while
    deciding to merge or not.

I hope that gives you a good idea of what next to do. :)

Damian Mooyman | Senior Platform Developer
SilverStripe
http://silverstripe.com/

Phone: +64 9 281 3498
Skype: tractorcow

On 5 May 2014 10:17, Ho0m3s notifications@github.com wrote:

I am new to GitHub so i probably messed up.
I don't understand your comments. I did not do anything to phpdoc. The fix
I am proposing is related to Virtual Page class.

If you try to overwrite the "VirtualPageMessage" LiteralField in a
DataExtension you end with 2 Literal Field the original and the one the
custom one. There is no way with the orginal VirutalPage class to remove
that original LiteralField, because it gets called twice. By applying what
you guys came up for bug#2827, to the VirtualPage i was able to customize
the Literal Field and remove the original.

The Fix i applied is the one you guys implemented in the
$member->getCMSField() with this->beforeUpdateCMS()... I just applied that
to the VirtualPage class.

I committed twice cause i left some debug stuff in the first. The meat of
the fix is found on the first commit. This is where you will see the
changes from the main branch. Is that what you mean by re-squash my
commits?

And also what do you mean by test case. It pretty easy to replicate.

Once again i am new at GitHub i looked at your documentation for
participating and it seemed pretty open. I am missing something? If so,
please let me know.


Reply to this email directly or view it on GitHubhttps://github.com/silverstripe/silverstripe-cms/pull/1008#issuecomment-42147396
.

tractorcow added the 3.1 label May 7, 2014

tractorcow added this to the 3.1.6 milestone May 7, 2014

dhensby closed this Nov 30, 2016

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