Add guards to decodeEntieties so it is not called with undefined. #6551

budzanowski · 2016-07-06T10:53:13Z

In initial loading phase code was calling decodeEntieties before data was available.
Now we check if the data is ready before the call.

To check:

open /desing
open /theme (info)
observe console

There should be no warnings regarding decodeEntieties being called with null, false or undefined.

Test live: https://calypso.live/?branch=fix/call_to_decodeentieties_with_undefined

ockham · 2016-07-07T16:27:40Z

I think you could try decodeEntities( param || '' ) instead. This way, the conditional is contained inside the argument, and using the fallback-or reads a bit better than the ternary IMO.

seear · 2016-07-12T12:15:22Z

This fix gets rid of the warning, but the problem remains that we are rendering the page without the proper title:

Could we connect my-sites/themes/head to the redux store and use the theme title and description when they become available instead of passing them in as props?

ockham · 2016-07-12T15:18:54Z

Could we connect my-sites/themes/head to the redux store

I'd rather not. Let's keep Head as dumb as possible for now and wait for #3795 to replace it properly.

Alternative suggestion: Let's drop makeElement, and move the <Head> wrapping to main.jsx -- as part of the component definition. That will connect() it. :-)

seear · 2016-07-28T11:12:11Z

client/my-sites/theme/controller.jsx


 const debug = debugFactory( 'calypso:themes' );
-let themeDetailsCache = new Map();
-
-export function makeElement( ThemesComponent, Head, store, props ) {


themes/controller.jsx is (was) using this function:

wp-calypso/client/my-sites/themes/controller.jsx

Line 15 in 6daec13

import { makeElement } from 'my-sites/theme/controller';

Ahh yes :( . Function added to themes/controller.jsx - it is just a helper function for that file now. Is there a reason that it should not be used this way(or in any way)? - @ockham wants to get rid of this function but i think it was horrible only in context of Theme Sheet, or there is more complicated reason?

I still want to get rid of it for themes lists too, but that can wait for another PR.

It's just a layer of indirection we don't really need any more. Ideally, we want some sort of generic adapter that connects a component to a routing middleware, and I had originally envisaged makeElement to be that (when it contained even more logic). However, I don't think so anymore; e.g. its signature isn't really good -- for example, it shouldn't have to accept Head as an arg.

But yeah, moving it to where it is actually used is fine for now.

ockham · 2016-07-29T10:46:29Z

client/my-sites/theme/main.jsx

@@ -345,33 +346,53 @@ const ThemeSheet = React.createClass( {
 		const analyticsPath = `/theme/:slug${ section ? '/' + section : '' }${ siteID ? '/:site_id' : '' }`;
 		const analyticsPageTitle = `Themes > Details Sheet${ section ? ' > ' + titlecase( section ) : '' }${ siteID ? ' > Site' : '' }`;

+		const Head = this.props.isLoggedIn
+			? require( 'layout/head' )
+			: require( 'my-sites/themes/head' );


I'd really love if we could get rid of the conditional import while we're here; I'm quite sure layout/head is all we ever need here. You can read its source (and compare to what fields we override here in main.jsx) to validate that assumption -- or maybe just try without and compare resulting title and metas. :-)

budzanowski · 2016-07-29T12:34:02Z

Meta and title from Head from layout looks OK.
TODO added.

ockham · 2016-07-29T12:48:19Z

Looking at my-sites/themes/head, I think there is one default defined there that we actually need here, too -- site_name. But I think that default value (WordPress.com) isn't themes specific anyway, so it should probably go to layout/head. (See the first couple of commits of #6814.)

ockham · 2016-07-29T12:49:40Z

client/my-sites/theme/main.jsx

+				type={ 'website' }
+				canonicalUrl={ canonicalUrl }
+				image={ this.props.screenshot }
+				tier={ this.props.tier || 'all' }>


No need for this prop.

ockham · 2016-08-01T12:20:22Z

client/my-sites/theme/main.jsx

@@ -345,33 +347,48 @@ const ThemeSheet = React.createClass( {
 		const analyticsPath = `/theme/:slug${ section ? '/' + section : '' }${ siteID ? '/:site_id' : '' }`;
 		const analyticsPageTitle = `Themes > Details Sheet${ section ? ' > ' + titlecase( section ) : '' }${ siteID ? ' > Site' : '' }`;

+		const themeName = this.props.name;


Just FYI, there's a neat shorthand for a construct like this,

const { name: themeName } = this.props;

ockham · 2016-08-01T13:28:05Z

I added 7ba1648 so we can really get rid of the decodeEntities( /* something*/ || '' ) fallbacks. Theme details will now be fetched as part of the fetchThemeDetailsData middleware on the client-side, too (unless they're already there, e.g. from initialState provided by the server). Would appreciate if you both could have a look @bb and @seear.

ockham · 2016-08-01T13:40:15Z

If you cherry-pick 31adaf0 and 2538eaa from #6814, I can close that one :-)

This reverts commit 7ba1648. We don't really want to wait for data to be fetched before rendering.

While it technically is required, this doesn't work with `ThemeMultiSiteSelector` (which clones its child element).

seear · 2016-08-01T14:34:49Z

client/my-sites/theme/main.jsx

@@ -73,7 +75,7 @@ const ThemeSheet = React.createClass( {
 			label: React.PropTypes.string.isRequired,
 			action: React.PropTypes.func,
 			getUrl: React.PropTypes.func,
-		} ).isRequired,


Cool, I was seeing warnings for this.

seear · 2016-08-01T14:35:43Z

Testing-wise this looks good to me. 👍

Merge when ready.

budzanowski added [Feature Group] Appearance & Themes Features related to the appearance of sites. [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jul 6, 2016

budzanowski added this to the Themes: Maintenance milestone Jul 6, 2016

budzanowski self-assigned this Jul 6, 2016

Add guards to decodeEntieties so it is not called with undefined.

a51751c

budzanowski force-pushed the fix/call_to_decodeentieties_with_undefined branch from 58c453d to a51751c Compare July 26, 2016 08:43

budzanowski added 6 commits July 26, 2016 13:08

First draft for connected Head in main.jsx

985df95

Remove decodeEntietis from SSR check

1658c61

Removing not used.

a143269

Correct linter warning and removed unused elements.

e23beac

Move Head to proper place.

7a31236

Linter warnings corrected.

4d85fa0

seear reviewed Jul 28, 2016
View reviewed changes

Move makeElement to place where it is used.

803ae8f

ockham reviewed Jul 29, 2016
View reviewed changes

Use Head from layout and add missing TODO

0dae8b5

ockham reviewed Jul 29, 2016
View reviewed changes

budzanowski and others added 2 commits July 29, 2016 22:45

Remove unnecessary prop and add default value for Header.

036411e

Head: Add propTypes

5c6956c

ockham reviewed Aug 1, 2016
View reviewed changes

ockham added 2 commits August 1, 2016 15:21

Themes: Pre-fill theme details on client side

7ba1648

Head: CamelCase siteName

61967f5

ockham added 3 commits August 1, 2016 16:12

Revert "Themes: Pre-fill theme details on client side"

715a8eb

This reverts commit 7ba1648. We don't really want to wait for data to be fetched before rendering.

Theme Sheet: Handle title and description fallbacks more gracefully

13c1285

Theme Sheet: Remove isRequired attr from defaultOption prop

ecc046a

While it technically is required, this doesn't work with `ThemeMultiSiteSelector` (which clones its child element).

seear reviewed Aug 1, 2016
View reviewed changes

seear added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Aug 1, 2016

budzanowski merged commit 9ea12f5 into master Aug 1, 2016

ockham mentioned this pull request Aug 1, 2016

Themes: Streamline Head usage #6814

Closed

budzanowski deleted the fix/call_to_decodeentieties_with_undefined branch August 2, 2016 01:11

lancewillett removed the [Status] Ready to Merge label Oct 19, 2016

This was referenced Mar 16, 2020

[Snyk] Security upgrade webpack from 3.4.1 to 4.0.0 anarh/wp-calypso#41

Open

[Snyk] Security upgrade webpack from 3.4.1 to 4.0.0 mdno/wp-calypso#48

Open

snyk-bot mentioned this pull request Jul 18, 2020

[Snyk] Fix for 13 vulnerabilities anarh/wp-calypso#47

Open

This was referenced Nov 17, 2020

[Snyk] Fix for 1 vulnerabilities anarh/wp-calypso#57

Open

[Snyk] Fix for 1 vulnerabilities mdno/wp-calypso#64

Open

This was referenced Mar 30, 2021

[Snyk] Security upgrade webpack from 3.4.1 to 4.0.0 anarh/wp-calypso#68

Open

[Snyk] Security upgrade webpack from 3.4.1 to 4.0.0 mdno/wp-calypso#74

Open

This was referenced Sep 15, 2021

[Snyk] Fix for 1 vulnerabilities anarh/wp-calypso#83

Open

[Snyk] Fix for 1 vulnerabilities mdno/wp-calypso#92

Open

This was referenced Apr 7, 2022

[Snyk] Fix for 1 vulnerabilities anarh/wp-calypso#94

Open

[Snyk] Fix for 1 vulnerabilities mayuba/wp-calypso#66

Open

[Snyk] Fix for 1 vulnerabilities mdno/wp-calypso#104

Open

[Snyk] Fix for 1 vulnerabilities NonProphets/wp-calypso#48

Open

mdno mentioned this pull request May 14, 2022

[Snyk] Fix for 1 vulnerabilities mdno/wp-calypso#105

Open

anarh mentioned this pull request May 14, 2022

[Snyk] Fix for 1 vulnerabilities anarh/wp-calypso#95

Open

mdno mentioned this pull request Jun 20, 2023

[Snyk] Fix for 1 vulnerabilities mdno/wp-calypso#123

Open

anarh mentioned this pull request Jun 21, 2023

[Snyk] Fix for 1 vulnerabilities anarh/wp-calypso#111

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add guards to decodeEntieties so it is not called with undefined. #6551

Add guards to decodeEntieties so it is not called with undefined. #6551

budzanowski commented Jul 6, 2016 •

edited by matticbot

Loading

ockham commented Jul 7, 2016

seear commented Jul 12, 2016 •

edited

Loading

ockham commented Jul 12, 2016

seear Jul 28, 2016

budzanowski Jul 29, 2016

ockham Jul 29, 2016

ockham Jul 29, 2016

budzanowski commented Jul 29, 2016

ockham commented Jul 29, 2016

ockham Jul 29, 2016

ockham Aug 1, 2016

ockham commented Aug 1, 2016

ockham commented Aug 1, 2016

seear Aug 1, 2016

seear commented Aug 1, 2016

Add guards to decodeEntieties so it is not called with undefined. #6551

Add guards to decodeEntieties so it is not called with undefined. #6551

Conversation

budzanowski commented Jul 6, 2016 • edited by matticbot Loading

ockham commented Jul 7, 2016

seear commented Jul 12, 2016 • edited Loading

ockham commented Jul 12, 2016

seear Jul 28, 2016

Choose a reason for hiding this comment

budzanowski Jul 29, 2016

Choose a reason for hiding this comment

ockham Jul 29, 2016

Choose a reason for hiding this comment

ockham Jul 29, 2016

Choose a reason for hiding this comment

budzanowski commented Jul 29, 2016

ockham commented Jul 29, 2016

ockham Jul 29, 2016

Choose a reason for hiding this comment

ockham Aug 1, 2016

Choose a reason for hiding this comment

ockham commented Aug 1, 2016

ockham commented Aug 1, 2016

seear Aug 1, 2016

Choose a reason for hiding this comment

seear commented Aug 1, 2016

budzanowski commented Jul 6, 2016 •

edited by matticbot

Loading

seear commented Jul 12, 2016 •

edited

Loading