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

Clarify Renderer discrepancy #4102

Merged
merged 3 commits into from Jan 16, 2021
Merged

Clarify Renderer discrepancy #4102

merged 3 commits into from Jan 16, 2021

Conversation

MGatner
Copy link
Member

@MGatner MGatner commented Jan 14, 2021

Description
Fixes a discrepancy between the interface docblock and the actual method definitions. Clarifies the usage in View.

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@MGatner
Copy link
Member Author

MGatner commented Jan 15, 2021

@WinterSilence @kenjis anything to add?

@WinterSilence
Copy link
Contributor

WinterSilence commented Jan 15, 2021

@MGatner
Define property saveData in ViewConfig:

class ViewConfig extends BaseConfig
{
     /**
      * @var boolean ...
      */
     public $saveData = false;
     ....
}

and replace

$this->saveData = $config->saveData ?? null;
to $this->saveData = (bool) $config->saveData;

Also, need fix @param array|null $options and $this->renderVars['options'] = $options ?? [];

@MGatner
Copy link
Member Author

MGatner commented Jan 15, 2021

@WinterSilence Thank you, all good fixes. Please re-review.

*
* @var boolean
*/
public $saveData = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

it's can be false by default - RendererInterface::render(string $view, array $options = null, bool $saveData = false)

Copy link
Member Author

Choose a reason for hiding this comment

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

It is true in app/Config/View.php which extends this, so I think it makes more sense to mirror that.

Copy link
Contributor

Choose a reason for hiding this comment

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

@MGatner why not same to RendererInterface::render()?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have no clue, I didn't write any of this. Probably a mistake or miscommunication among earlier devs. At this point though we have a public interface which we can't change and an App config file which we could change but would need to note in docs - I think keeping the config files consistent even though they don't match the interface makes the most sense given the mess it already is.

@MGatner MGatner merged commit b28cd47 into codeigniter4:develop Jan 16, 2021
@MGatner MGatner deleted the renderer branch January 16, 2021 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: classes overwrites parameter defaults, phpDoc's defined in interfaces
4 participants