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

Change params variable to reflect merged state #16971

Merged
merged 3 commits into from Jul 6, 2017

Conversation

Projects
None yet
@matrikular
Contributor

matrikular commented Jul 4, 2017

Pull Request for Issue #16968.

Summary of Changes

Change params variable to reflect merged state (in address sub-layout).

Testing Instructions

See #16968 (comment)

Expected result

The mentioned field values and icons are shown to the user.

Actual result

They are not.

@richard67

This comment has been minimized.

Show comment
Hide comment
@richard67

richard67 Jul 4, 2017

Contributor

I have tested this item successfully on 8d0e5f5


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16971.

Contributor

richard67 commented Jul 4, 2017

I have tested this item successfully on 8d0e5f5


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16971.

@bembelimen

This comment has been minimized.

Show comment
Hide comment
@bembelimen

bembelimen Jul 4, 2017

Contributor

I have tested this item successfully on 8d0e5f5


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16971.

Contributor

bembelimen commented Jul 4, 2017

I have tested this item successfully on 8d0e5f5


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16971.

@richard67

This comment has been minimized.

Show comment
Hide comment
@richard67

richard67 Jul 4, 2017

Contributor

It seems drone failed for reasons not related to this PR (some phpcs script missing).

Contributor

richard67 commented Jul 4, 2017

It seems drone failed for reasons not related to this PR (some phpcs script missing).

@gorgonz

This comment has been minimized.

Show comment
Hide comment
@gorgonz

gorgonz Jul 4, 2017

I saw the bug after updating. Gave it a try by exchanging my override with this new version in three different websites. Works fine now :-)

gorgonz commented Jul 4, 2017

I saw the bug after updating. Gave it a try by exchanging my override with this new version in three different websites. Works fine now :-)

@infograf768 infograf768 removed the PR-staging label Jul 5, 2017

@infograf768

This comment has been minimized.

Show comment
Hide comment
@infograf768
Member

infograf768 commented Jul 5, 2017

rtc


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16971.

@joomla-cms-bot joomla-cms-bot added the RTC label Jul 5, 2017

@infograf768 infograf768 added this to the Joomla 3.7.4 milestone Jul 5, 2017

@AlexRed

This comment has been minimized.

Show comment
Hide comment
@AlexRed

AlexRed Jul 5, 2017

Contributor

I think we need same edit also in the template beez override
templates/beez3/html/com_contact/contact/default_address.php

Contributor

AlexRed commented Jul 5, 2017

I think we need same edit also in the template beez override
templates/beez3/html/com_contact/contact/default_address.php

@lunalars

This comment has been minimized.

Show comment
Hide comment
@lunalars

lunalars Jul 5, 2017

Contributor

hm, if all template overrides have to be updated, isn't it a b/c break then? in other words: something was changed and now breaks template overrides.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16971.

Contributor

lunalars commented Jul 5, 2017

hm, if all template overrides have to be updated, isn't it a b/c break then? in other words: something was changed and now breaks template overrides.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16971.

@dgrammatiko

This comment has been minimized.

Show comment
Hide comment
@dgrammatiko

dgrammatiko Jul 5, 2017

Member

if all template overrides have to be updated, isn't it a b/c break then

Not according to the B/C promise:

6.1.7 Rendered markup

For the time being, rendered markup is not subject to our backwards compatibility promise. We will try not to change markup in such a way that a site might render differently, but we can't promise not to break anything at the present time. We will work on defining ways in which we might make a backwards compatibility promise for markup in the future, but we do not currently have a satisfactory consensus on a workable standard.

Member

dgrammatiko commented Jul 5, 2017

if all template overrides have to be updated, isn't it a b/c break then

Not according to the B/C promise:

6.1.7 Rendered markup

For the time being, rendered markup is not subject to our backwards compatibility promise. We will try not to change markup in such a way that a site might render differently, but we can't promise not to break anything at the present time. We will work on defining ways in which we might make a backwards compatibility promise for markup in the future, but we do not currently have a satisfactory consensus on a workable standard.

@lunalars

This comment has been minimized.

Show comment
Hide comment
@lunalars

lunalars Jul 5, 2017

Contributor

ah, ok. thanks for the info!

Contributor

lunalars commented Jul 5, 2017

ah, ok. thanks for the info!

@dgrammatiko

This comment has been minimized.

Show comment
Hide comment
@dgrammatiko

dgrammatiko Jul 5, 2017

Member

@lunalars actually the mark up with this PR remains the same so this is just a bug fix and it shouldn't affect you unless you already have created an override with the latest version

Member

dgrammatiko commented Jul 5, 2017

@lunalars actually the mark up with this PR remains the same so this is just a bug fix and it shouldn't affect you unless you already have created an override with the latest version

@lunalars

This comment has been minimized.

Show comment
Hide comment
@lunalars

lunalars Jul 5, 2017

Contributor

@dgt41 I am affected, but I'm not sure why. Maybe I'm doing somthing wrong in a plugin I created, which adds some fields to the contact edit form in backend. These fields are saved with the params of the contact.
In my template overrides I was able to get these params with
$this->params->get('paramname'),
but now I have to use
$this->item->params->get('paramname') or $this->contact->params->get('paramname').
This only applies to the "custom" params, the "core" params work as before.

Now I'm confused :-) But maybe I should just add my fields in a different way.

Edit: And yes, my problem is not related to this PR

Contributor

lunalars commented Jul 5, 2017

@dgt41 I am affected, but I'm not sure why. Maybe I'm doing somthing wrong in a plugin I created, which adds some fields to the contact edit form in backend. These fields are saved with the params of the contact.
In my template overrides I was able to get these params with
$this->params->get('paramname'),
but now I have to use
$this->item->params->get('paramname') or $this->contact->params->get('paramname').
This only applies to the "custom" params, the "core" params work as before.

Now I'm confused :-) But maybe I should just add my fields in a different way.

Edit: And yes, my problem is not related to this PR

@zero-24 zero-24 removed the RTC label Jul 5, 2017

@joomla-cms-bot joomla-cms-bot removed this from the Joomla 3.7.4 milestone Jul 5, 2017

@zero-24

This comment has been minimized.

Show comment
Hide comment
@zero-24

zero-24 Jul 5, 2017

Contributor

I'm switching back to pending based on a special request by @matrikular


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16971.

Contributor

zero-24 commented Jul 5, 2017

I'm switching back to pending based on a special request by @matrikular


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16971.

@matrikular

This comment has been minimized.

Show comment
Hide comment
@matrikular

matrikular Jul 5, 2017

Contributor

Thanks @zero-24. I'll post an update soon (this evening).

Contributor

matrikular commented Jul 5, 2017

Thanks @zero-24. I'll post an update soon (this evening).

@matrikular

This comment has been minimized.

Show comment
Hide comment
@matrikular

matrikular Jul 5, 2017

Contributor

First of all, I feel responsible for this and do want to make it right. You can be assured, that it is not my intention to hide behind some "yet unwritten", "debatable", "between the lines" interpretation of the stated b/c promise regarding rendered markup.

That said, personally - I would expect every site owner / admin / webmaster to thoroughly check their overrides after each update and that a layout is hardly to keep free from "breaking" changes. A simple change in the data layer or parameter handling e.g. can very well affect the output.

So much for cheap excuses,...
I also think, in case of the address layout, it would be (already is) a b/c break in a way that, everyone that already has got an override, would need to update it because of the "new" variable being used. This is because in different from the default and custom fields layout, it uses $this->params variable directly, where the other two accessed the parameters via the variable $tparams or $params, therefore no layout wide renaming was needed.

I've reverted the changes back to the original state for the address layout and assigned the merged parameters to $this->params in the view. That way, there should be no need to change an already existing override. Well, at least not in this context (hopefully).

Some more tests are highly appreciated, thanks.

Contributor

matrikular commented Jul 5, 2017

First of all, I feel responsible for this and do want to make it right. You can be assured, that it is not my intention to hide behind some "yet unwritten", "debatable", "between the lines" interpretation of the stated b/c promise regarding rendered markup.

That said, personally - I would expect every site owner / admin / webmaster to thoroughly check their overrides after each update and that a layout is hardly to keep free from "breaking" changes. A simple change in the data layer or parameter handling e.g. can very well affect the output.

So much for cheap excuses,...
I also think, in case of the address layout, it would be (already is) a b/c break in a way that, everyone that already has got an override, would need to update it because of the "new" variable being used. This is because in different from the default and custom fields layout, it uses $this->params variable directly, where the other two accessed the parameters via the variable $tparams or $params, therefore no layout wide renaming was needed.

I've reverted the changes back to the original state for the address layout and assigned the merged parameters to $this->params in the view. That way, there should be no need to change an already existing override. Well, at least not in this context (hopefully).

Some more tests are highly appreciated, thanks.

@richard67

This comment has been minimized.

Show comment
Hide comment
@richard67

richard67 Jul 5, 2017

Contributor

I have tested this item successfully on 597a503

@matrikular Just when you had posted your previous comment I have finishing my test ;-)


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16971.

Contributor

richard67 commented Jul 5, 2017

I have tested this item successfully on 597a503

@matrikular Just when you had posted your previous comment I have finishing my test ;-)


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16971.

@franz-wohlkoenig

This comment has been minimized.

Show comment
Hide comment
@franz-wohlkoenig

franz-wohlkoenig Jul 5, 2017

I have tested this item successfully on 597a503


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16971.

franz-wohlkoenig commented Jul 5, 2017

I have tested this item successfully on 597a503


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16971.

@franz-wohlkoenig

This comment has been minimized.

Show comment
Hide comment
@franz-wohlkoenig

franz-wohlkoenig Jul 5, 2017

RTC after two successful tests.

franz-wohlkoenig commented Jul 5, 2017

RTC after two successful tests.

@joomla-cms-bot joomla-cms-bot added the RTC label Jul 5, 2017

@lunalars

This comment has been minimized.

Show comment
Hide comment
@lunalars

lunalars Jul 5, 2017

Contributor

I have tested this item successfully on 597a503

Fixes problem mentioned in #16968 and custom overrides.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16971.

Contributor

lunalars commented Jul 5, 2017

I have tested this item successfully on 597a503

Fixes problem mentioned in #16968 and custom overrides.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16971.

@bayareajenn

This comment has been minimized.

Show comment
Hide comment
@bayareajenn

bayareajenn Jul 5, 2017

I have tested this item successfully on 597a503

I tested this on four sites that had this issue and all tested great with the fix provided. Thank you!
Jenn


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16971.

bayareajenn commented Jul 5, 2017

I have tested this item successfully on 597a503

I tested this on four sites that had this issue and all tested great with the fix provided. Thank you!
Jenn


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16971.

@infograf768 infograf768 added this to the Joomla 3.7.4 milestone Jul 6, 2017

@rdeutz rdeutz merged commit 877381d into joomla:staging Jul 6, 2017

5 of 6 checks passed

continuous-integration/jenkins/pr-merge The build of this commit was aborted
Details
JTracker/HumanTestResults Human Test Results: 4 Successful 0 Failed.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
hound No violations found. Woof!

@joomla-cms-bot joomla-cms-bot added PR-staging and removed RTC labels Jul 6, 2017

@matrikular matrikular deleted the matrikular:patch-19 branch Jul 6, 2017

@Sandra97 Sandra97 referenced this pull request Jul 12, 2017

Closed

Contact Us Page v3.7.3 #992

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