Shorter copyright header

[sreichel]

Description (*)

No need to mention license again.

• closes License docblocks, can we make it better (and smaller)? #3137

[Axepih]

Hi,

What do you think of what piwigo has done?
Piwigo/Piwigo@fc193f7

The copyright is available in a single file COPYING.txt
https://github.com/Piwigo/Piwigo/blob/master/COPYING.txt

[justinbeaty]

I would agree IDEs are more important than a static documentation website, but I do like both. 😃

I'm back at my computer now, so to be clear here's an example, but I don't have a current PHPStorm license to check how it shows.

<?php
/**
 * Base html block
 *
 * Example usage, etc...
 * 
 * @category   Mage
 * @package    Mage_Core
 * @copyright  This file is part of OpenMage. For copyright and license information, please view the COPYING.txt file that was distributed with this source code.
 * @license    Open Software License (OSL 3.0)
 *
 * @method $this setContentHeading(string $value)
 * @method $this setDestElementId(string $value)
 * @method $this setFormAction(string $value)
 * @method $this setIdSuffix(string $value)
 * @method $this setProduct(Mage_Catalog_Model_Product $value)
 * @method $this setDisplayMinimalPrice(bool $value)
 */
class Mage_Core_Block_Template extends Mage_Core_Block_Abstract
{

Edit: I lengthened the @copyright tag, but my point was that the copyright message shouldn't be a part of any docblock's title or description since it's neither of those things.

[colinmollenhour]

Thanks for the example, that looks great to me, a nice improvement. If you agree, do you mind applying that method to class files @sreichel ? No changes needed for all other files (phtml, css, etc).

[justinbeaty]

I think there is room for improvement on the phtml files too, for example:

https://github.com/sreichel/magento-lts/blob/a4742a5674c4132bacf2189c7f2fbd3938821117/app/design/adminhtml/default/default/template/eav/attribute/options.phtml#L1-L20

It could be something like:

<?php
/**
 * Attribute manage options form
 *
 * This template is responsible for rendering the form at Catalog > Attributes > Manage Attributes > Manage Labels / Options tab.
 * You can customize it by... (etc)
 * 
 * @category    design
 * @package     default_default
 * @copyright   This file is part of OpenMage[...]
 * @license     Academic Free License (AFL 3.0)
 */

/** @var Mage_Eav_Block_Adminhtml_Attribute_Edit_Options_Abstract $this */
?>
<div>
<ul class="messages">
    <li class="notice-msg">
<!-- ... -->

I'm not tied to any format specific format or wording either. Whatever is decided we should test on various IDEs. It would be nice to make this the final docblock update.

[justinbeaty]

And my memory could be hazy, so I'll confirm my findings before @sreichel were to change anything in this PR. I would like to hear other people's opinions too. 😃

[sreichel]

I'm not opposed to adding it back, is that hard to do @sreichel? I'm not sure what method you're using to make all of these changes.

Really often I use "my time".

The PR was about to remove some "duplicate" content.

Not touching the years. Not touching the copyrights ...

[justinbeaty]

I had a chance to double check things with the official phpdoc package. Regardless of the outcome of this PR, here are some more notes:

1. The @category tag is deprecated:

This was necessary since the @package tag, as defined in the original Standard, did not allow for more than one hierarchy level. Since this has changed this tag SHOULD NOT be used.
Source: https://docs.phpdoc.org/guide/references/phpdoc/tags/category.html

2. Just the @package tag is needed, and the values we already have for those tags are fine. They could be more specific, i.e. Mage_Catalog_Model instead of just Mage_Catalog, but it feels unnecessary for now.

3. Using the @package (base|rwd|default)_default tag in phtml files seems pointless to me. Template files don't define any classes, functions, or global constants, so there's nothing to associate into the namespace. Instead the documentation site shows three namespaces (base_default, rwd_default, and default_default) but they all just show an empty table of contents. If anything, I believe the @package value should refer to the module that the template file belongs to, so @package Mage_Page for the file template/page/2columns-left.phtml.

Expand screenshot

---

Updated docblock template proposal

Not a final docblock template by any means, just updating the previous post to remove @category and from all docblocks, and additionally remove @package from phtml files.

# app/code/core/Mage/Core/Block/Template.php
<?php 

/**
 * Base html block
 *
 * Example usage, etc. Can use markdown:
 * ```php
 * $filename = Mage::getDesign()->getTemplateFilename($this->getTemplate(), $params);
 * ```
 *
 * @package    Mage_Core
 * @copyright  This file is part of OpenMage. For copyright and license information, please view the COPYING.txt file that was distributed with this source code.
 * @license    Open Software License (OSL 3.0)
 *
 * @method $this setContentHeading(string $value)
 * @method $this setDestElementId(string $value)
 * @method $this setFormAction(string $value)
 * @method $this setIdSuffix(string $value)
 * @method $this setProduct(Mage_Catalog_Model_Product $value)
 * @method $this setDisplayMinimalPrice(bool $value)
 */
class Mage_Core_Block_Template extends Mage_Core_Block_Abstract

# app/design/adminhtml/default/default/template/eav/attribute/options.phtml
<?php

/**
 * Attribute manage options form
 *
 * This template is responsible for rendering the form at Catalog > Attributes > Manage Attributes > Manage Labels / Options tab.
 * You can customize it by... (etc)
 *
 * @copyright   This file is part of OpenMage. For copyright and license information, please view the COPYING.txt file that was distributed with this source code.
 * @license     Academic Free License (AFL 3.0)
 */

/** @var Mage_Eav_Block_Adminhtml_Attribute_Edit_Options_Abstract $this */
?>
<div>
<ul class="messages">

---

Screenshots

Here's some screenshots of how each option (current main branch, this PR, and my proposed docblock) render.

1. Class: Mage_Core_Block_Template

This is the page that shows the Class Definition of Mage_Core_Block_Template. Both the current and this PR render the same, so I only show two screenshots.

The current screenshot shows a useless "category: Mage" tag when we already know we're in the Mage package. There is also no copyright or license information. Unrelated, but I added some markdown to that file in the proposed version just to show what's possible.

Expand screenshots

2. File: app/code/core/Mage/Core/Block/Template.php
This is the page that shows the File Level metadata in the file where Mage_Core_Block_Template is defined. Personally I think the proposed screenshot looks the best, and IMO there's no need to show copyright info here, because the @copyright is attached to the class.

Expand screenshots

3. File: app/design/adminhtml/default/default/template/eav/attribute/options.phtml
This is just a template file, but both current and this PR screenshots lack any information about the file, even though there is this docblock, because that second docblock doesn't attach to anything, except potentially a variable defined after it.

<?php
/**
 * OpenMage
 *
 * This source file is subject to the Academic Free License (AFL 3.0)
 * that is bundled with this package in the file LICENSE_AFL.txt.
 * It is also available at https://opensource.org/license/afl-3-0-php
 *
 * @category    design
 * @package     default_default
 * @copyright   Copyright (c) 2006-2020 Magento, Inc. (https://www.magento.com)
 * @copyright   Copyright (c) 2021-2024 The OpenMage Contributors (https://www.openmage.org)
 * @license     https://opensource.org/licenses/afl-3.0.php  Academic Free License (AFL 3.0)
 */
?>
<?php
/**
 * Attribute options control
 *
 * @see Mage_Eav_Block_Adminhtml_Attribute_Edit_Options_Abstract
 * @var Mage_Eav_Block_Adminhtml_Attribute_Edit_Options_Abstract $this
 */
?>

Expand screenshots

[addison74]

I support moving most of the copyright into a root file. Just a small comment related to this line

"@copyright This file is part of OpenMage. For copyright and license information, please view the COPYING.txt file that was distributed with this source code."

If it is required make it smaller, for example

"@copyright For copyright and license information please read the COPYING.txt file."

[colinmollenhour]

I like all the proposed changes by @justinbeaty and @addison74.

Sorry if I hijacked the original PR purpose, @sreichel, but I think it was only after you added the workflow to update the copyright years so I didn't think that was outside of the scope. I was just asking about what method you were using in case you had a smarter method than mine so I could help. I'd probably just use a the obvious "Find and Replace" personally, but I'm sure there are other more clever ways.. I'll make the changes if you prefer me to do it, just let me know.

[justinbeaty]

@colinmollenhour have you tried those updated docblocks in your IDE? Presumably you only see the class docblocks and not the file level docblocks anywhere, so just want to make sure those look good on your end.

I should also check if putting the @var ... $this and any @see tags in the file level docblock works. It is a bit cleaner, but I don't know what others think about it. For example:

<?php

/**
 * Adminhtml manage attribute options form block
 *
 * Long description...
 *
 * @var         Mage_Eav_Block_Adminhtml_Attribute_Edit_Options_Abstract $this  <-- This line
 * @see         Some value...
 * @copyright   For copyright and license information please read the COPYING.txt file
 * @license     Academic Free License (AFL 3.0)
 */
?>

[colinmollenhour]

I think for @var it should be in its own comment for best IDE compatibility. Are there other IDEs besides PhpStorm and VS Code (and plugins/variants) that are relevant these days?

[sreichel]

@justinbeaty @colinmollenhour @addison74 thanks for suggestions.

To reproduce output i've added phpdocumentor.

To not run it run for whole code you can add this to root.

Takes 10s to have docs at http://localhost:63342/magento-lts/build/phpdoc-test/

phpdoc.xml

<?xml version="1.0" encoding="UTF-8" ?>
<phpdocumentor
        configVersion="3"
        xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
        xmlns="https://www.phpdoc.org"
        xsi:noNamespaceSchemaLocation="https://raw.githubusercontent.com/phpDocumentor/phpDocumentor/master/data/xsd/phpdoc.xsd"
>
    <setting name="template.color" value="blue"/>

    <paths>
        <output>build/phpdoc-test/api</output>
        <cache>build/phpdoc-test/cache</cache>
    </paths>

    <version number="3.0.0">
        <api>
            <source dsn=".">
                <path>app/code/core/Mage/Admin</path>
                <path>COPYING.txt</path>
            </source>
            <extensions>
                <extension>php</extension>
                <extension>txt</extension>
            </extensions>
            <visibility>public</visibility>
            <visibility>protected</visibility>
            <visibility>private</visibility>
        </api>
    </version>
</phpdocumentor>

Run

vendor/bin/phpdoc run

---

From my tests ...

• phpDocumentor does not render class-blocks correctly when file-block exists. So it makes sense to combine it, like Shorter copyright header #4467 (comment).
• category-tag can be removed See Shorter copyright header #4467 (comment)

For php-files we have this atm? Added link ...

/**
 * Class description
 *
 * @copyright   For copyright and license information read COPYING.txt file
 * @copyright   ... additional info ....
 * @link        /COPYING.txt
 * @package     Mage_Xml
 * 
 * ... methods, etc ...
 */
class Some_Class

(note: slash is requird to linkt to copying.txt)

@colinmollenhour i am happy for every improvent, but its bit wiered ... there are other PRs that deserve more attention.

I have used simple "find&replace", later with regex pattern. Now i think about a script to automate all suggestion ... lol. "Problem" is, it takes time to process 5k files (again and again). Espaccially running on old hardware (core-i3, 16gb 😢)

I'll make the changes if you prefer me to do it, just let me know.

As for every PR, pleae fork and submit a PR to improve.

PS: can you please prepare a subdomain to publish phpdocumentor?

[colinmollenhour]

Yeah, as often happens, a small suggestion becomes a bigger one... But I think we arrived at a good solution.

What subdomain do you want? phpdoc.openmage.org?

[sreichel]

I looks good, but how to do it all files?

How to move class descripion on top, put methods below copright?

You are fine with regex, any ideas for complex replace pattern?

phpdoc.openmage.org is good 👍

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[colinmollenhour]

I used a script generated by AI to make the changes. I think it worked, but it needs more review as it's possible there are some mistakes.. Not sure how to go about reviewing 5000 files. See #4624

[sreichel]

thanks for thinking about us, since our project has many unique parts that can't be labeled as "copyright by openmage"

Whats sooo unique?

• you did a f*** great job with replacing prototype.js
• justin made some good changes to autoloader
• ...
• ... but majoity of your commits comes from here

Why not to put that energy into a "RWD v2" or "Maho"-Theme? Why add CLI commands instead of supporting n98?

[sreichel]

thanks for thinking about us, since our project has many unique parts that can't be labeled as "copyright by openmage"

I'd like to add the Maho-theme as addition to RWD., so we need to place copyrights in phtmls too.

[colinmollenhour]

Whats sooo unique?

@sreichel This is why there are not enough maintainers. 😞

[sreichel]

See #4624