Jump to content
Wanze

SeoMaestro

Recommended Posts

5 hours ago, Mikie said:

Hi @Wanze, getting an error with 0.8.0 update with the structured data breadcrumb template using TemplateLatteReplace module. Its fine as I can ignore that file with Latte using the module settings, but maybe changes relying on rendering templates like this should default to unchecked, as I assume same issue would occur with other template systems.

SeoMaestro (including myself) knows nothing about TemplateLatteReplace, so I cannot influence settings of this module. I am not sure why the LatteReplace would try to render the structured data template, I assume it's because SeoMaestro internally uses a ProcessWire TemplateFile to render the markup: https://github.com/wanze/SeoMaestro/blob/master/src/StructuredData/BreadcrumbStructuredData.php#L52-L55

There is nothing I can do from Seo Maestro's side about this, and I assume you will have problems with other modules as well that are using ProcessWire's TemplateFile for rendering. My TemplateEngineFactory module providing template engines like Twig, Smarty, Pug etc. does not suffer from this problem, because it hooks after Pager::render rather than TemplateFile::render.

Cheers

  • Like 4

Share this post


Link to post
Share on other sites
18 hours ago, Wanze said:

SeoMaestro (including myself) knows nothing about TemplateLatteReplace, so I cannot influence settings of this module. I am not sure why the LatteReplace would try to render the structured data template, I assume it's because SeoMaestro internally uses a ProcessWire TemplateFile to render the markup: https://github.com/wanze/SeoMaestro/blob/master/src/StructuredData/BreadcrumbStructuredData.php#L52-L55

There is nothing I can do from Seo Maestro's side about this, and I assume you will have problems with other modules as well that are using ProcessWire's TemplateFile for rendering. My TemplateEngineFactory module providing template engines like Twig, Smarty, Pug etc. does not suffer from this problem, because it hooks after Pager::render rather than TemplateFile::render.

Hi @Wanze, I was just suggesting for new functionality maybe it is best to default to off initially, eg for this update have Breadcrumb unchecked by default and allow users to opt in and test. Nothing to do with controlling other modules settings. My rationale for this is that since this module directly renders output, updates that add functionality are essentially breaking in that they affect rendered html. I would think only essential updates that are needed for the module to function would default on. Just a thought.

You are right re TemplateLatteReplace, This line:
https://github.com/rolandtoth/TemplateLatteReplace/blob/cbac28b93e74e3ba51f7ce32f374c385e406d09a/TemplateLatteReplace.module#L186
checks against current page template not the template called by the TemplateFile class. This is fixable by getting the basename from $templateFile->filename instead, will submit an issue to @tpr.

Thanks for the response!

  • Like 1

Share this post


Link to post
Share on other sites

Hey @Wanze! These aren't really issues, more general feedback, so I thought I'd post them here. I've been evaluating this module as a solution for handling metadata, and as such have finally had some time to actually dig into it's numerous features – and while I see a lot of interesting stuff here, there are a few things that could perhaps use some polishing, in my opinion 🙂

So one thing that confused me initially was the use of word "inherit". I managed to miss the point about default values being set template level in README (which obviously was a major reason for my confusion... but to my defence: when evaluating a new module I like to do it without reading too much first, since that's how most regular users – as in clients – are going to be using it anyway), so I imagined that the values would be inherited from parent pages. (Seemed a bit weird obviously, but whatever.)

Now that I've double-checked the docs and understand that those values are inherited from the template, I'm actually wondering if you might consider calling this something else – perhaps "default"? It would also be good to explain somehow (in context) what "inherit" means, and where the value is inherited from?

Note: I get that anyone installing the module and having access to template settings should probably realise what this is all about, but I for one don't give clients access to template settings (I see that as the developers' territory). Thus "inherit" doesn't make much sense to them – and since it's not explained in the admin (and they're unlikely to figure out that the site is using SeoMaestro and dig out the modules README or this thread), it can indeed be quite confusing.

--

Another source of confusion for me was the Sitemap feature, mainly because it didn't seem to do anything. Now, looking into the source code, I see that you're using file_put_contents() – so apparently the module expects write access to the site root? It might be a good idea to add a check to see if this really is the case. At least I assume that this was the problem – didn't see any errors, but in my case Apache or PHP never have write access to directories with executable code, so if the module did try to write in those directories, it must've failed.

(Personally I like the "hook 404 page and serve fake files" approach more when it comes to things like this – it doesn't require write access, and tends to work better overall in different environments.)

--

Just some initial observations – hope you don't mind poking around. You're doing great job with this module! 🙂

  • Like 8

Share this post


Link to post
Share on other sites

Hi @teppo

Thanks for your valuable feedback!

On 7/2/2019 at 10:21 PM, teppo said:

Now that I've double-checked the docs and understand that those values are inherited from the template, I'm actually wondering if you might consider calling this something else – perhaps "default"? It would also be good to explain somehow (in context) what "inherit" means, and where the value is inherited from?

What do you think about calling the label "Inherit default value"? For the explanation, we could use an InputfieldMarkup prior to any meta data fields (similar to the one when editing the default values on field-level).

It has been mentioned before that it should be possible for content editors to edit default values without having permission to edit fields or templates. I think the module will (should) include another Process module, which could offer this configuration GUI, along with other interesting things such as SEO reports 😉

On 7/2/2019 at 10:21 PM, teppo said:

Another source of confusion for me was the Sitemap feature, mainly because it didn't seem to do anything. Now, looking into the source code, I see that you're using file_put_contents() – so apparently the module expects write access to the site root? It might be a good idea to add a check to see if this really is the case. At least I assume that this was the problem – didn't see any errors, but in my case Apache or PHP never have write access to directories with executable code, so if the module did try to write in those directories, it must've failed.

(Personally I like the "hook 404 page and serve fake files" approach more when it comes to things like this – it doesn't require write access, and tends to work better overall in different environments.)

The sitemap does not have to be in the site root necessarily, you could also enter a path to a writeable folder. But you're right, I actually never thought of this problem and also do not check it 😅 At least this should be mentioned in a description or note. I am aware of the 404 hook, but I do not like it very much, it feels "wrong". Other hooks listening to the 404's might get triggered as well, but then it's no longer a 404 after returning the sitemap. Let me think about this 😄 

Cheers

  • Like 3

Share this post


Link to post
Share on other sites
23 hours ago, Wanze said:

What do you think about calling the label "Inherit default value"? For the explanation, we could use an InputfieldMarkup prior to any meta data fields (similar to the one when editing the default values on field-level).

It has been mentioned before that it should be possible for content editors to edit default values without having permission to edit fields or templates. I think the module will (should) include another Process module, which could offer this configuration GUI, along with other interesting things such as SEO reports 😉

All of these sound like great ideas 👍

23 hours ago, Wanze said:

The sitemap does not have to be in the site root necessarily, you could also enter a path to a writeable folder. But you're right, I actually never thought of this problem and also do not check it 😅 At least this should be mentioned in a description or note. I am aware of the 404 hook, but I do not like it very much, it feels "wrong". Other hooks listening to the 404's might get triggered as well, but then it's no longer a 404 after returning the sitemap. Let me think about this 😄 

Sure, no worries. I get that there are ups and downs to both approaches.

My use case may be a bit unorthodox, but again I never give ProcessWire or the Apache user write access to anywhere that isn't strictly necessary (so only the assets directory, or parts of it). In my opinion giving PHP write access to executable code is asking for trouble. Additionally ProcessWire writing files to the disk can be problematic if you're using automated deployment process – depending on your setup those files may get lost in every deployment, or at least require some extra logic to work properly.

Just some points to consider, and of course folks like me can always leave the sitemap feature to another module or custom code. In my opinion the minimum level would be checking if the file can be added, and then somehow warning the user if it can't. Just for reference, here's how I ended up implementing a similar permission check 🙂

  • Like 2

Share this post


Link to post
Share on other sites

HI! Awesome module @Wanze!

Does anyone know if this module allows a fallback field  (or lists of them) within the page if for example, the description is not set?

Share this post


Link to post
Share on other sites

Thanks @elabx

The module allows to fallback to fields using placeholders. For example, if your page has a field lead_text, you can set the default value of the meta description to {lead_text}. If the content editor omits the meta description of a page, it will fallback to the lead text. It is not possible to fallback to multiple fields though. Does this answer your question?

Cheers

  • Like 1

Share this post


Link to post
Share on other sites

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.

Guest
Reply to this topic...

×   Pasted as rich text.   Paste as plain text instead

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.


  • Recently Browsing   0 members

    No registered users viewing this page.

×
×
  • Create New...